feat: support concurrent chunk uploads#125
Conversation
|
Closing in favor of focused PR #126. |
Greptile SummaryThis PR adds concurrent chunk uploads to the Android SDK by dispatching up to 8 simultaneous
Confidence Score: 3/5The concurrency logic has correctness gaps that could silently corrupt an upload or return stale metadata to the caller in production. The core of the change is structurally sound, but two captured mutable variables ( library/src/main/java/io/appwrite/Client.kt — the rewritten Important Files Changed
Reviews (1): Last reviewed commit: "feat: support concurrent chunk uploads" | Re-trigger Greptile |
| if (isUploadComplete(chunkResult)) { | ||
| result = chunkResult | ||
| } |
There was a problem hiding this comment.
Non-volatile
result written from concurrent async blocks
result is a plain var captured by up to 8 concurrent async coroutines, each of which may independently evaluate isUploadComplete and write to it without synchronization. On a multi-threaded dispatcher (e.g. Dispatchers.IO), the JVM does not guarantee that the write from one coroutine is ever visible to another without a @Volatile annotation or an AtomicReference. In the worst case, result may still hold the first-chunk response (or the pre-upload current state in a resume path) after awaitAll() if none of the concurrent writes are flushed. The caller on line 662 would then convert stale metadata as if the upload were complete. Mark result as @Volatile or use AtomicReference<Map<*,*>?> to ensure the final write is visible.
|
|
||
| if (includeUploadId && uploadId != null) { | ||
| chunkHeaders["x-appwrite-id"] = uploadId!! | ||
| } | ||
|
|
||
| chunkHeaders["Content-Range"] = "bytes $start-${end - 1}/$size" | ||
| chunkParams[paramName] = MultipartBody.Part.createFormData( | ||
| paramName, | ||
| input.filename, | ||
| buffer.toRequestBody() | ||
| readChunk(start, end).toRequestBody() | ||
| ) | ||
|
|
||
| headers["Content-Range"] = | ||
| "bytes $offset-${((offset + CHUNK_SIZE) - 1).coerceAtMost(size - 1)}/$size" | ||
|
|
||
| result = call( | ||
| val chunkResult = call( | ||
| method = "POST", | ||
| path, | ||
| headers, | ||
| params, | ||
| chunkHeaders, | ||
| chunkParams, | ||
| responseType = Map::class.java | ||
| ) | ||
|
|
||
| offset += CHUNK_SIZE | ||
| headers["x-appwrite-id"] = result["\$id"].toString() | ||
| if (index == 0 || uploadId == null) { | ||
| uploadId = chunkResult["\$id"].toString() | ||
| } |
There was a problem hiding this comment.
uploadId is not @Volatile but is read across coroutine threads
uploadId is written in the sequential section (chunk 0 upload or from params[idParamName]) and then read by every concurrent uploadChunk invocation at the if (includeUploadId && uploadId != null) check. Without @Volatile, the JVM does not guarantee that a write performed on one thread is visible to coroutines dispatched to other threads, even if the write happened-before the coroutineScope { } call in program order. If a concurrent coroutine reads a stale null, it silently omits the x-appwrite-id header; the server would then treat the chunk as a new file creation rather than an additional chunk, silently corrupting or duplicating the upload. Declaring uploadId as @Volatile costs nothing and eliminates the risk.
| onProgress?.invoke( | ||
| UploadProgress( | ||
| id = uploadId ?: chunkResult["\$id"].toString(), | ||
| progress = sizeUploaded.coerceAtMost(size).toDouble() / size * 100, | ||
| sizeUploaded = sizeUploaded.coerceAtMost(size), | ||
| chunksTotal = chunkResult["chunksTotal"].toString().toInt(), | ||
| chunksUploaded = chunksUploaded, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
onProgress invoked concurrently from multiple coroutines
Up to 8 async blocks can call onProgress?.invoke(...) simultaneously on different threads. If the callback updates shared state (e.g. a UI progress bar, a counter, or a LiveData) without its own synchronization, those updates will race. Additionally, because chunk completion ordering is non-deterministic, a callback for a later-indexed chunk may fire before one for an earlier-indexed chunk, so callers that assume monotonically ordered delivery will observe unexpected behaviour. Documenting that this callback may be invoked concurrently from background threads, or funnelling all invocations through a single serialised dispatcher, would make the contract explicit.
This PR updates the SDK to support concurrent chunk uploads.