Skip to content

feat: support concurrent chunk uploads#125

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

feat: support concurrent chunk uploads#125
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.

@TorstenDittmann
Copy link
Copy Markdown
Contributor Author

Closing in favor of focused PR #126.

@greptile-apps
Copy link
Copy Markdown

greptile-apps Bot commented May 21, 2026

Greptile Summary

This PR adds concurrent chunk uploads to the Android SDK by dispatching up to 8 simultaneous async coroutines (controlled by MAX_CONCURRENT_UPLOADS) and coordinating progress with AtomicInteger/AtomicLong. It also refactors file reading to open a fresh RandomAccessFile per chunk (enabling safe parallel seeks) and correctly sizes the last chunk's byte buffer.

  • The shared result and uploadId variables are plain vars captured by up to 8 concurrent async blocks; without @Volatile the JVM does not guarantee cross-thread write visibility, so the final upload response or the upload ID could appear stale to a concurrent coroutine.
  • The onProgress callback is invoked concurrently from multiple background threads with no documented thread-safety contract, which can cause races in any caller that updates shared or UI state.

Confidence Score: 3/5

The 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 (result and uploadId) are shared across coroutines without @Volatile. If a concurrent coroutine reads a stale null uploadId, it omits the x-appwrite-id header and the server treats that chunk as a new file creation rather than an additional chunk. If result is never visibly updated, the caller receives the first-chunk response as the completed upload metadata. Both failure paths are silent.

library/src/main/java/io/appwrite/Client.kt — the rewritten chunkedUpload method, specifically the uploadId/result variable declarations and the concurrent upload dispatch block.

Important Files Changed

Filename Overview
library/src/main/java/io/appwrite/Client.kt Rewrites chunkedUpload to dispatch up to 8 concurrent async chunk uploads; introduces non-volatile shared variables (result, uploadId) written/read across coroutine threads, and an unsynchronised onProgress callback invocation pattern.
README.md Version bump from 24.1.1 to 24.2.0 in Gradle and Maven installation snippets — straightforward, no issues.

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

Comment on lines +643 to +645
if (isUploadComplete(chunkResult)) {
result = chunkResult
}
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 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.

Comment on lines +573 to +595

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()
}
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 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.

Comment on lines +647 to +655
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,
)
)
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 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.

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