Skip to content

Commit 3866f38

Browse files
tsdicloudjuliusknorr
tsdicloud
and
juliusknorr
committed
Refactor writeObject to only use MultipartUpload when required
Signed-off-by: Bernd Rederlechner <Bernd.Rederlechner@t-systems.com> Co-authored-by: Julius Härtl <jus@bitgrid.net>
1 parent 98e2dce commit 3866f38

File tree

4 files changed

+166
-20
lines changed

4 files changed

+166
-20
lines changed

‎lib/private/Files/ObjectStore/S3ConnectionTrait.php‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -74,9 +74,9 @@ protected function parseParams($params) {
7474

7575
$this->test = isset($params['test']);
7676
$this->bucket = $params['bucket'];
77-
$this->proxy = isset($params['proxy']) ? $params['proxy'] : false;
78-
$this->timeout = !isset($params['timeout']) ? 15 : $params['timeout'];
79-
$this->uploadPartSize = !isset($params['uploadPartSize']) ? 524288000 : $params['uploadPartSize'];
77+
$this->proxy = $params['proxy'] ?? false;
78+
$this->timeout = $params['timeout'] ?? 15;
79+
$this->uploadPartSize = $params['uploadPartSize'] ?? 524288000;
8080
$params['region'] = empty($params['region']) ? 'eu-west-1' : $params['region'];
8181
$params['hostname'] = empty($params['hostname']) ? 's3.' . $params['region'] . '.amazonaws.com' : $params['hostname'];
8282
if (!isset($params['port']) || $params['port'] === '') {

‎lib/private/Files/ObjectStore/S3ObjectTrait.php‎

Lines changed: 58 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,10 +28,11 @@
2828

2929
use Aws\S3\Exception\S3MultipartUploadException;
3030
use Aws\S3\MultipartUploader;
31-
use Aws\S3\ObjectUploader;
3231
use Aws\S3\S3Client;
33-
use Icewind\Streams\CallbackWrapper;
32+
use GuzzleHttp\Psr7\Utils;
3433
use OC\Files\Stream\SeekableHttpStream;
34+
use GuzzleHttp\Psr7;
35+
use Psr\Http\Message\StreamInterface;
3536

3637
trait S3ObjectTrait {
3738
/**
@@ -80,37 +81,77 @@ public function readObject($urn) {
8081
}
8182

8283
/**
84+
* Single object put helper
85+
*
8386
* @param string $urn the unified resource name used to identify the object
84-
* @param resource $stream stream with the data to write
87+
* @param StreamInterface $stream stream with the data to write
8588
* @param string|null $mimetype the mimetype to set for the remove object @since 22.0.0
8689
* @throws \Exception when something goes wrong, message will be logged
87-
* @since 7.0.0
8890
*/
89-
public function writeObject($urn, $stream, string $mimetype = null) {
90-
$count = 0;
91-
$countStream = CallbackWrapper::wrap($stream, function ($read) use (&$count) {
92-
$count += $read;
93-
});
91+
protected function writeSingle(string $urn, StreamInterface $stream, string $mimetype = null): void {
92+
$this->getConnection()->putObject([
93+
'Bucket' => $this->bucket,
94+
'Key' => $urn,
95+
'Body' => $stream,
96+
'ACL' => 'private',
97+
'ContentType' => $mimetype,
98+
]);
99+
}
94100

95-
$uploader = new MultipartUploader($this->getConnection(), $countStream, [
101+
102+
/**
103+
* Multipart upload helper that tries to avoid orphaned fragments in S3
104+
*
105+
* @param string $urn the unified resource name used to identify the object
106+
* @param StreamInterface $stream stream with the data to write
107+
* @param string|null $mimetype the mimetype to set for the remove object
108+
* @throws \Exception when something goes wrong, message will be logged
109+
*/
110+
protected function writeMultiPart(string $urn, StreamInterface $stream, string $mimetype = null): void {
111+
$uploader = new MultipartUploader($this->getConnection(), $stream, [
96112
'bucket' => $this->bucket,
97113
'key' => $urn,
98114
'part_size' => $this->uploadPartSize,
99115
'params' => [
100116
'ContentType' => $mimetype
101-
]
117+
],
102118
]);
103119

104120
try {
105121
$uploader->upload();
106122
} catch (S3MultipartUploadException $e) {
107-
// This is an empty file so just touch it then
108-
if ($count === 0 && feof($countStream)) {
109-
$uploader = new ObjectUploader($this->getConnection(), $this->bucket, $urn, '');
110-
$uploader->upload();
111-
} else {
112-
throw $e;
123+
// if anything goes wrong with multipart, make sure that you don´t poison and
124+
// slow down s3 bucket with orphaned fragments
125+
$uploadInfo = $e->getState()->getId();
126+
if ($e->getState()->isInitiated() && (array_key_exists('UploadId', $uploadInfo))) {
127+
$this->getConnection()->abortMultipartUpload($uploadInfo);
113128
}
129+
throw $e;
130+
}
131+
}
132+
133+
134+
/**
135+
* @param string $urn the unified resource name used to identify the object
136+
* @param resource $stream stream with the data to write
137+
* @param string|null $mimetype the mimetype to set for the remove object @since 22.0.0
138+
* @throws \Exception when something goes wrong, message will be logged
139+
* @since 7.0.0
140+
*/
141+
public function writeObject($urn, $stream, string $mimetype = null) {
142+
$psrStream = Utils::streamFor($stream);
143+
144+
// ($psrStream->isSeekable() && $psrStream->getSize() !== null) evaluates to true for a On-Seekable stream
145+
// so the optimisation does not apply
146+
$buffer = new Psr7\Stream(fopen("php://memory", 'rwb+'));
147+
Utils::copyToStream($psrStream, $buffer, MultipartUploader::PART_MIN_SIZE);
148+
$buffer->seek(0);
149+
if ($buffer->getSize() < MultipartUploader::PART_MIN_SIZE) {
150+
// buffer is fully seekable, so use it directly for the small upload
151+
$this->writeSingle($urn, $buffer, $mimetype);
152+
} else {
153+
$loadStream = new Psr7\AppendStream([$buffer, $psrStream]);
154+
$this->writeMultiPart($urn, $loadStream, $mimetype);
114155
}
115156
}
116157

‎tests/lib/Files/ObjectStore/ObjectStoreTest.php‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,11 +26,27 @@
2626

2727
abstract class ObjectStoreTest extends TestCase {
2828

29+
/** @var string[] */
30+
private $cleanup = [];
31+
2932
/**
3033
* @return \OCP\Files\ObjectStore\IObjectStore
3134
*/
3235
abstract protected function getInstance();
3336

37+
protected function cleanupAfter(string $urn) {
38+
$this->cleanup[] = $urn;
39+
}
40+
41+
public function tearDown(): void {
42+
parent::tearDown();
43+
44+
$instance = $this->getInstance();
45+
foreach ($this->cleanup as $urn) {
46+
$instance->deleteObject($urn);
47+
}
48+
}
49+
3450
protected function stringToStream($data) {
3551
$stream = fopen('php://temp', 'w+');
3652
fwrite($stream, $data);
@@ -110,6 +126,9 @@ public function testExists() {
110126
}
111127

112128
public function testCopy() {
129+
$this->cleanupAfter('source');
130+
$this->cleanupAfter('target');
131+
113132
$stream = $this->stringToStream('foobar');
114133

115134
$instance = $this->getInstance();

‎tests/lib/Files/ObjectStore/S3Test.php‎

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,12 @@ public function stream_seek($offset, $whence = SEEK_SET) {
6060
* @group PRIMARY-s3
6161
*/
6262
class S3Test extends ObjectStoreTest {
63+
public function setUp(): void {
64+
parent::setUp();
65+
$s3 = $this->getInstance();
66+
$s3->deleteObject('multiparttest');
67+
}
68+
6369
protected function getInstance() {
6470
$config = \OC::$server->getConfig()->getSystemValue('objectstore');
6571
if (!is_array($config) || $config['class'] !== S3::class) {
@@ -70,6 +76,8 @@ protected function getInstance() {
7076
}
7177

7278
public function testUploadNonSeekable() {
79+
$this->cleanupAfter('multiparttest');
80+
7381
$s3 = $this->getInstance();
7482

7583
$s3->writeObject('multiparttest', NonSeekableStream::wrap(fopen(__FILE__, 'r')));
@@ -80,6 +88,8 @@ public function testUploadNonSeekable() {
8088
}
8189

8290
public function testSeek() {
91+
$this->cleanupAfter('seek');
92+
8393
$data = file_get_contents(__FILE__);
8494

8595
$instance = $this->getInstance();
@@ -94,4 +104,80 @@ public function testSeek() {
94104
fseek($read, 100, SEEK_CUR);
95105
$this->assertEquals(substr($data, 210, 100), fread($read, 100));
96106
}
107+
108+
public function assertNoUpload($objectUrn) {
109+
$s3 = $this->getInstance();
110+
$s3client = $s3->getConnection();
111+
$uploads = $s3client->listMultipartUploads([
112+
'Bucket' => $s3->getBucket(),
113+
'Prefix' => $objectUrn,
114+
]);
115+
$this->assertArrayNotHasKey('Uploads', $uploads);
116+
}
117+
118+
public function testEmptyUpload() {
119+
$s3 = $this->getInstance();
120+
121+
$emptyStream = fopen("php://memory", "r");
122+
fwrite($emptyStream, null);
123+
124+
$s3->writeObject('emptystream', $emptyStream);
125+
126+
$this->assertNoUpload('emptystream');
127+
$this->assertTrue($s3->objectExists('emptystream'));
128+
129+
$thrown = false;
130+
try {
131+
self::assertFalse($s3->readObject('emptystream'));
132+
} catch (\Exception $e) {
133+
// An exception is expected here since 0 byte files are wrapped
134+
// to be read from an empty memory stream in the ObjectStoreStorage
135+
$thrown = true;
136+
}
137+
self::assertTrue($thrown, 'readObject with range requests are not expected to work on empty objects');
138+
139+
$s3->deleteObject('emptystream');
140+
}
141+
142+
/** File size to upload in bytes */
143+
public function dataFileSizes() {
144+
return [
145+
[1000000], [2000000], [5242879], [5242880], [5242881], [10000000]
146+
];
147+
}
148+
149+
/** @dataProvider dataFileSizes */
150+
public function testFileSizes($size) {
151+
$this->cleanupAfter('testfilesizes');
152+
$s3 = $this->getInstance();
153+
154+
$sourceStream = fopen('php://memory', 'wb+');
155+
$writeChunkSize = 1024;
156+
$chunkCount = $size / $writeChunkSize;
157+
for ($i = 0; $i < $chunkCount; $i++) {
158+
fwrite($sourceStream, str_repeat('A',
159+
($i < $chunkCount - 1) ? $writeChunkSize : $size - ($i * $writeChunkSize)
160+
));
161+
}
162+
rewind($sourceStream);
163+
$s3->writeObject('testfilesizes', $sourceStream);
164+
165+
$this->assertNoUpload('testfilesizes');
166+
self::assertTrue($s3->objectExists('testfilesizes'));
167+
168+
$result = $s3->readObject('testfilesizes');
169+
170+
// compare first 100 bytes
171+
self::assertEquals(str_repeat('A', 100), fread($result, 100));
172+
173+
// compare 100 bytes
174+
fseek($result, $size - 100);
175+
self::assertEquals(str_repeat('A', 100), fread($result, 100));
176+
177+
// end of file reached
178+
fseek($result, $size);
179+
self:self::assertTrue(feof($result));
180+
181+
$this->assertNoUpload('testfilesizes');
182+
}
97183
}

0 commit comments

Comments
 (0)