Skip to content

improve handling of large single-part s3 uploads #49352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
juliusknorr merged 4 commits into from
Dec 6, 2024
Merged

improve handling of large single-part s3 uploads #49352

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7da8951
fix: use php://temp instead of php://memory for multi-part upload buffer
icewind1991 Nov 18, 2024
ca547bb
fix: don't perform the extra buffering in s3 stream write when the st…
icewind1991 Nov 18, 2024
e4d0882
fix: improve assembly stream
icewind1991 Nov 27, 2024
6cf66f9
fix: get object size from stream where possible
icewind1991 Dec 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: improve assembly stream 
Signed-off-by: Robin Appelman <[email protected]>
  • Loading branch information
@icewind1991
icewind1991 committed Dec 5, 2024
commit e4d0882b312923ee9021312c5272df4884276961
22 changes: 14 additions & 8 deletions apps/dav/lib/Upload/AssemblyStream.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,10 @@ public function stream_seek($offset, $whence = SEEK_SET) {
$offset = $this->size + $offset;
}

if ($offset === $this->pos) {
return true;
}

if ($offset > $this->size) {
return false;
}
Expand All @@ -95,7 +99,7 @@ public function stream_seek($offset, $whence = SEEK_SET) {

$stream = $this->getStream($this->nodes[$nodeIndex]);
$nodeOffset = $offset - $nodeStart;
if (fseek($stream, $nodeOffset) === -1) {
if ($nodeOffset > 0 && fseek($stream, $nodeOffset) === -1) {
return false;
}
$this->currentNode = $nodeIndex;
Expand Down Expand Up @@ -126,9 +130,14 @@ public function stream_read($count) {
}
}

do {
$collectedData = '';
// read data until we either got all the data requested or there is no more stream left
while ($count > 0 && !is_null($this->currentStream)) {
$data = fread($this->currentStream, $count);
$read = strlen($data);

$count -= $read;
$collectedData .= $data;
$this->currentNodeRead += $read;

if (feof($this->currentStream)) {
Expand All @@ -145,14 +154,11 @@ public function stream_read($count) {
$this->currentStream = null;
}
}
// if no data read, try again with the next node because
// returning empty data can make the caller think there is no more
// data left to read
} while ($read === 0 && !is_null($this->currentStream));
}

// update position
$this->pos += $read;
return $data;
$this->pos += strlen($collectedData);
return $collectedData;
}

/**
Expand Down
22 changes: 19 additions & 3 deletions apps/dav/tests/unit/Upload/AssemblyStreamTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,16 @@ public function testGetContents($expected, $nodes): void {
/**
* @dataProvider providesNodes()
*/
public function testGetContentsFread($expected, $nodes): void {
public function testGetContentsFread($expected, $nodes, $chunkLength = 3): void {
$stream = AssemblyStream::wrap($nodes);

$content = '';
while (!feof($stream)) {
$content .= fread($stream, 3);
$chunk = fread($stream, $chunkLength);
$content .= $chunk;
if ($chunkLength !== 3) {
$this->assertEquals($chunkLength, strlen($chunk));
}
}

$this->assertEquals($expected, $content);
Expand Down Expand Up @@ -103,7 +107,19 @@ public function providesNodes() {
]],
'a ton of nodes' => [
$tonofdata, $tonofnodes
]
],
'one read over multiple nodes' => [
'1234567890', [
$this->buildNode('0', '1234'),
$this->buildNode('1', '5678'),
$this->buildNode('2', '90'),
], 10],
'two reads over multiple nodes' => [
'1234567890', [
$this->buildNode('0', '1234'),
$this->buildNode('1', '5678'),
$this->buildNode('2', '90'),
], 5],
];
}

Expand Down