Skip to content

feat: support concurrent chunk uploads#74

Closed
TorstenDittmann wants to merge 1 commit into
mainfrom
concurrent-chunk-uploads-1-9-x
Closed

feat: support concurrent chunk uploads#74
TorstenDittmann wants to merge 1 commit into
mainfrom
concurrent-chunk-uploads-1-9-x

Conversation

@TorstenDittmann
Copy link
Copy Markdown
Contributor

This PR updates the SDK to support concurrent chunk uploads.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR adds concurrent chunk uploads to the Storage, Functions, and Sites services by replacing the sequential while loop with a curl_multi-based approach that sends up to 8 chunks in parallel per batch. It also adds createOAuth2Session, createPushTarget, updatePushTarget, and deletePushTarget to the Account service, removes three deprecated Deno runtimes from the BuildRuntime and Runtime enums, and drops the prior 24.1.0 changelog entry.

  • Concurrent upload (Storage/Functions/Sites): Each upload now dispatches the first chunk sequentially to obtain the upload ID, then processes all remaining chunks in batches of 8 using raw curl_multi_* calls. Error handling and progress tracking are wired for each batch, though curl handles are not explicitly closed after removal from the multi handle, and no cleanup runs when an exception aborts a batch mid-loop.
  • Account additions: Four new methods follow existing SDK patterns and are covered by new unit tests.

Confidence Score: 3/5

The new concurrent upload path works correctly on the happy path but leaks curl resources when a chunk upload fails mid-batch; long-running workers doing large uploads could exhaust file descriptors over time.

All three upload service files share the same curl handle cleanup gap: when AppwriteException is thrown inside the foreach loop, curl_multi_close is never called and unprocessed handles are abandoned. In high-throughput or long-running PHP-FPM environments this can deplete file descriptors. Additionally, Storage.php has a redundant dead-code initialisation, and the header serialisation in the concurrent path omits the conventional space after the colon, diverging from the sequential path.

src/Appwrite/Services/Storage.php, src/Appwrite/Services/Functions.php, and src/Appwrite/Services/Sites.php all share the same concurrent upload implementation and need the curl cleanup and header-format fixes reviewed before merging.

Important Files Changed

Filename Overview
src/Appwrite/Services/Storage.php Refactored chunk upload to use curl_multi for concurrent batches of 8; contains dead-code double $uploadId initialisation and leaks curl handles when an exception is thrown mid-batch.
src/Appwrite/Services/Functions.php Same concurrent curl_multi upload refactoring as Storage.php; shares the curl handle leak on exception and the missing space in the header serialisation loop.
src/Appwrite/Services/Sites.php Identical concurrent upload refactoring copied from Functions.php, including the same curl handle and header format issues.
src/Appwrite/Services/Account.php Added createOAuth2Session, createPushTarget, updatePushTarget, and deletePushTarget methods; straightforward additions following existing SDK patterns.
src/Appwrite/Enums/BuildRuntime.php Removed deprecated Deno 1.21, 1.24, and 1.35 runtime entries from both the enum factory methods and the string-to-enum map.
src/Appwrite/Enums/Runtime.php Mirrors BuildRuntime.php: removes the same three deprecated Deno runtime entries.
tests/Appwrite/Services/AccountTest.php Adds unit tests for the four new Account methods; consistent with existing mock-based test style.
CHANGELOG.md Removes the 24.1.0 entry entirely; may be intentional if this PR targets a new version number.

Reviews (1): Last reviewed commit: "feat: support concurrent chunk uploads" | Re-trigger Greptile

Comment on lines +586 to +624
foreach ($handles as $handleInfo) {
$ch = $handleInfo['handle'];
$body = curl_multi_getcontent($ch);
$statusCode = curl_getinfo($ch, CURLINFO_HTTP_CODE);
$contentType = $responseHeaders[spl_object_id($ch)]['content-type'] ?? '';

if (curl_errno($ch)) {
throw new AppwriteException(curl_error($ch), $statusCode, '', $body);
}

$chunkResponse = str_starts_with($contentType, 'application/json') ? json_decode($body, true) : $body;

if($statusCode >= 400) {
if(is_array($chunkResponse)) {
throw new AppwriteException($chunkResponse['message'], $statusCode, $chunkResponse['type'] ?? '', json_encode($chunkResponse));
}

throw new AppwriteException($chunkResponse, $statusCode, '', $chunkResponse);
}

$completedCount++;
$uploadedSize += $handleInfo['chunk']['end'] - $handleInfo['chunk']['start'];
if($isUploadComplete($chunkResponse)) {
$response = $chunkResponse;
}
if($onProgress !== null) {
$onProgress([
'$id' => $uploadId,
'progress' => $uploadedSize / $size * 100,
'sizeUploaded' => $uploadedSize,
'chunksTotal' => $totalChunks,
'chunksUploaded' => $completedCount,
]);
}

curl_multi_remove_handle($multiHandle, $ch);
}

curl_multi_close($multiHandle);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 curl handles never explicitly closed after removal

After curl_multi_remove_handle($multiHandle, $ch), the individual curl handles are never closed with curl_close($ch). In the normal path the multi handle's reference to each handle is released when curl_multi_close is called, allowing eventual GC. However when an exception is thrown inside the foreach loop (either the curl_errno branch or the $statusCode >= 400 branch), both the multi handle and all remaining individual handles are abandoned without cleanup. In long-running PHP-FPM workers or CLI scripts with large multi-chunk uploads, this can exhaust available file descriptors. The same pattern appears in Functions.php and Sites.php.

Comment on lines +446 to +448
$uploadId = '';
$uploadId = $fileId ?? '';
$totalChunks = (int) ceil($size / Client::CHUNK_SIZE);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Dead code: $uploadId = '' on the first line is immediately overwritten by the assignment on the next line and has no effect. This appears to be a copy-paste artefact from Functions.php (which has no corresponding $fileId initialisation line).

Suggested change
$uploadId = '';
$uploadId = $fileId ?? '';
$totalChunks = (int) ceil($size / Client::CHUNK_SIZE);
$uploadId = $fileId ?? '';
$totalChunks = (int) ceil($size / Client::CHUNK_SIZE);

Comment on lines +723 to +726
$headers = [];
foreach ($chunkHeaders as $key => $value) {
$headers[] = $key . ':' . $value;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 HTTP header format missing space after colon

Headers are assembled as key . ':' . value (no space), while the conventional and Client::call()-consistent form is key . ': ' . value. Most servers tolerate both, but some strict reverse-proxies or WAFs reject the no-space variant, meaning the concurrent upload path could produce 400/502 responses that the sequential first-chunk path would not. The same pattern appears in Sites.php and Storage.php.

@TorstenDittmann
Copy link
Copy Markdown
Contributor Author

Closing in favor of focused PR #75.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant