Skip to content

feat: support concurrent chunk uploads#75

Merged
TorstenDittmann merged 2 commits into
mainfrom
concurrent-chunk-uploads-1-9-x-minimal
May 21, 2026
Merged

feat: support concurrent chunk uploads#75
TorstenDittmann merged 2 commits into
mainfrom
concurrent-chunk-uploads-1-9-x-minimal

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 refactors the chunked upload paths in Functions.php, Sites.php, and Storage.php to upload the first chunk sequentially (to obtain the upload ID), then dispatch remaining chunks in concurrent batches of 8 using curl_multi. The event-loop correctly handles CURLM_CALL_MULTI_PERFORM, and a try/finally block ensures all handles are closed on both success and failure paths.

  • The $uploadChunk closure and the inner $makeHandle closure duplicate the same request-assembly logic (headers, content-range, x-appwrite-id); a single shared helper would reduce the maintenance surface across the three identical files.
  • The $id variable initialized at the top of the upload block in Functions.php and Sites.php is never read after the refactor — $uploadId has taken its place, leaving $id as dead code.

Confidence Score: 4/5

The concurrent upload machinery is functionally sound — handle cleanup, the multi-exec event loop, and upload-ID propagation all work correctly — but the three files are identical copies of non-trivial curl_multi code, and the split between the sequential first-chunk path and the concurrent path leaves duplicated request-building logic that could drift silently.

The critical correctness issues from the previous review round (CURLM_CALL_MULTI_PERFORM handling, resource cleanup on exception) have been addressed. The remaining findings are maintenance concerns rather than runtime defects, but the volume of duplicated low-level networking code across three files means a subtle future mistake is more likely to go unnoticed.

All three service files carry identical curl_multi logic; Functions.php and Sites.php also retain the now-unused $id variable from the old sequential loop.

Important Files Changed

Filename Overview
src/Appwrite/Services/Functions.php Sequential first-chunk bootstrap + curl_multi batching (8 at a time) for remaining chunks; try/finally cleans up handles correctly; CURLM_CALL_MULTI_PERFORM handled; dead $id variable and duplicated request-assembly logic left behind from the old loop.
src/Appwrite/Services/Sites.php Identical structure to Functions.php; same dead $id variable and duplicated $uploadChunk/$makeHandle logic.
src/Appwrite/Services/Storage.php Same concurrent-upload pattern; $uploadId correctly seeded from $fileId for resumable uploads; dead $id variable and double $uploadId = '' initialisation remain (latter flagged in a prior review thread).

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

Comment thread src/Appwrite/Services/Functions.php Outdated
Comment thread src/Appwrite/Services/Functions.php Outdated
Comment thread src/Appwrite/Services/Storage.php
@TorstenDittmann TorstenDittmann merged commit 8463031 into main May 21, 2026
1 check passed
@TorstenDittmann TorstenDittmann deleted the concurrent-chunk-uploads-1-9-x-minimal branch May 21, 2026 17:53
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.

2 participants