Repository navigation
Keep an upload pending until its completion has finished - #457
Open
didiergarcia wants to merge 2 commits into
Open
didiergarcia wants to merge 2 commits into
didiergarcia wants to merge 2 commits into
Conversation
URLSession marks a task .completed before running its completion, and cleanupUploads freed the batch as soon as the task stopped running. A flush in that window selected the same file again; the first completion then deleted it, so the second upload sent an empty body (400). Uploads now carry a flag their completion sets when it's done, and HTTPClient always calls an upload's completion, including when the client was released mid-upload.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
9 open issues
UploadTaskInfo.completiondefaults to a newCompletion()instance, which makes it easy for… · Newcompletion.finish()is called both in adeferand again immediately beforecleanupUploads().… · Newcompletion.finish()is called both in adeferand again immediately beforecleanupUploads().… · Newcompletion.finish()is called both in adeferand again immediately beforecleanupUploads().… · Newcompletion.finish()is called both in adeferand again immediately beforecleanupUploads().… · Newresponseisn’t anHTTPURLResponse, the code now completes with.badSessionbut does not… · NewRunLoopdelay to allow async work to complete, which can introduce… · New
What changed in this PR
Prevents duplicate batch uploads and flush deadlocks by keeping uploads “pending” until their completion handler has finished processing the batch, and by ensuring HTTPClient always invokes the caller completion even if the client is released mid-upload.
Changes:
- Track upload completion-finalization via a shared
Completionflag and updatecleanupUploads()removal rules accordingly. - Ensure
HTTPClientcalls the upload completion whenselfis released and when the response isn’t anHTTPURLResponse. - Add/adjust tests to reproduce and verify the race (real
URLSessionUploadTaskcompletion window) and the released-client completion case.
| File | Description |
|---|---|
| Tests/Segment-Tests/UploadOwnership_Tests.swift | Adds regression tests for “completed-but-completion-still-running” and released-HTTPClient behaviors. |
| Tests/Segment-Tests/Support/TestUtilities.swift | Updates test session task state handling to be atomic/thread-safe. |
| Tests/Segment-Tests/SegmentDestination_Tests.swift | Updates/extends cleanupUploads() tests for the new completion-finalization rule. |
| Sources/Segment/Utilities/Networking/HTTPClient.swift | Ensures completion is always called (including when client is deallocated or response type is unexpected). |
| Sources/Segment/Plugins/SegmentDestination.swift | Introduces per-upload completion-finalization tracking and updates upload cleanup logic to match. |
Review effort: Lite
Comment on lines
+48
to
+54
| // set by the upload's completion once it has finished with the batch. | ||
| var completion = Completion() | ||
|
|
||
| final class Completion { | ||
| @Atomic var finished = false | ||
| func finish() { _finished.set(true) } | ||
| } |
Comment on lines
189
to
193
| let uploadTask = httpClient.startBatchUpload(writeKey: analytics.configuration.values.writeKey, batch: url) { [weak self] result in | ||
| defer { | ||
| completion.finish() | ||
| leaveGroup.run() | ||
| } |
Comment on lines
+217
to
218
| completion.finish() | ||
| cleanupUploads() |
Comment on lines
299
to
304
| let uploadTask = httpClient.startBatchUpload(writeKey: analytics.configuration.values.writeKey, data: data, batchId: batchId) { [weak self] result in | ||
| defer { | ||
| completion.finish() | ||
| // leave for the url we kicked off. | ||
| finish.run() | ||
| } |
Comment on lines
+327
to
328
| completion.finish() | ||
| cleanupUploads() |
| } else { | ||
| completion(.failure(HTTPClientErrors.statusCode(code: httpResponse.statusCode))) | ||
| } | ||
| } else { |
Comment on lines
+157
to
+158
| // let A's completion and any second upload finish. | ||
| RunLoop.main.run(until: Date(timeIntervalSinceNow: 1.0)) |
| @@ -204,12 +214,15 @@ extension SegmentDestination { | |||
| // the upload we have here has just finished. | |||
| // make sure it gets removed and it's cleanup() called rather | |||
| // lets go through and get rid of any tasks that aren't running. | ||
| // either they were suspended because a background task took too | ||
| // long, or the os orphaned it due to device constraints (like a watch). | ||
| // lets go through and get rid of uploads that are done: their completion has finished |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #456.
cleanupUploads()removed an upload as soon as its task stopped reporting.running, andflush()only selects batches when no uploads are pending. A realURLSessionUploadTaskis already.completedwhen its completion handler starts, and the handler is what deletes the batch file. A flush in that window selected the same file again; the first completion then deleted it, so the second upload sent an empty body and got a 400 (or, if it read the file first, the batch was sent twice).cleanupUploads()removes an upload when that flag is set, or when the task is.suspendedand won't complete. The flag is created before the upload starts, since its completion can run before the upload is added. Disk and memory paths both use it.HTTPClientnow always calls an upload's completion. Settings can replace the client mid-upload, and the response handler returned without calling it once the client was released (also when the response wasn't anHTTPURLResponse). Those cases complete with.badSession, which keeps the batch for retry. Without this, an upload could stay pending forever and block later flushes.A custom
HTTPSessionwhose tasks report.completedwithout ever calling their completion would now keep that upload pending. URLSession always calls it.Verified:
UploadOwnership_Testsuses real URLSession tasks and holds the first completion while a second flush runs. Onmainthe same file is uploaded twice with a 0-byte second body; with the fix it's uploaded once. The released-client test times out onmainand passes with the fix. Both passed 20/20 repeats.cleanupUploadsunit tests updated for the new rule, plus cases for a completed task whose completion hasn't finished and a suspended task.sdk-e2e-testsmain run locally against this branch: 90 passed, 3 skipped, 0 failed.