Prevent duplicate Storage requests on resume - #16771
dajiaohuang wants to merge 2 commits into
Conversation
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. |
|
Thanks for the contribution! A few housekeeping items:
|
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request prevents duplicate upload and download requests when resuming a task before its initial setup completes by introducing generation tracking (enqueueGeneration) in StorageDownloadTask and StorageUploadTask. Unit tests have been added to verify that earlier setups are discarded upon resumption. The review feedback highlights a potential data race in both task implementations where setupDiscardedHandlerForTesting is accessed outside of the lock, and suggests synchronizing this access by retrieving the handler under the lock before executing it.
| guard stateLock.withLock({ generation == enqueueGeneration }) else { | ||
| setupDiscardedHandlerForTesting?() | ||
| return | ||
| } |
There was a problem hiding this comment.
Accessing setupDiscardedHandlerForTesting outside of the lock while checking generation == enqueueGeneration introduces a potential data race. Since StorageTask is marked @unchecked Sendable and uses manual locking, all shared mutable state accesses must be synchronized. We should retrieve the handler under the lock and then execute it outside the lock to ensure thread safety.
let discardedHandler = stateLock.withLock { () -> (@Sendable () -> Void)? in
guard generation == enqueueGeneration else {
return setupDiscardedHandlerForTesting
}
return nil
}
if let discardedHandler {
discardedHandler()
return
}| guard stateLock.withLock({ generation == enqueueGeneration }) else { | ||
| setupDiscardedHandlerForTesting?() | ||
| return | ||
| } |
There was a problem hiding this comment.
Accessing setupDiscardedHandlerForTesting outside of the lock while checking generation == enqueueGeneration introduces a potential data race. Since StorageTask is marked @unchecked Sendable and uses manual locking, all shared mutable state accesses must be synchronized. We should retrieve the handler under the lock and then execute it outside the lock to ensure thread safety.
let discardedHandler = stateLock.withLock { () -> (@Sendable () -> Void)? in
guard generation == enqueueGeneration else {
return setupDiscardedHandlerForTesting
}
return nil
}
if let discardedHandler {
discardedHandler()
return
}
paulb777
left a comment
paulb777
left a comment
There was a problem hiding this comment.
Thanks, this is easy to hit: putData(...) already enqueues, so an immediate resume() (or a quick pause()/resume()) runs setup twice and starts two fetchers, orphaning the first. The generation approach looks sound to me.
CI confirms one blocker: testUploadResumeDiscardsEarlierSetup crashes with Fatal error: Internal error enqueueing a Storage task in the SPM unit tests (e.g. spm (xcode-27, catalyst)). The cause is inline.
Coverage: please add tests for the most likely real-world trigger (putData(...).resume() and getData(...).resume() right after creation, with no pause()), and for download resume() during .progress.
| await StorageFetcherService.shared.updateServiceWaiterForTesting(nil) | ||
| } | ||
|
|
||
| let task = rootReference().putData(Data("upload".utf8)) |
There was a problem hiding this comment.
rootReference() has no object path, so uploadMetadata.path stays nil and the resumed setup hits the fatalError at StorageUploadTask.swift:107. That's the crash in CI. rootReference().child("object") should fix it.
| XCTAssertTrue(task.hasFetcherForTesting) | ||
|
|
||
| await serviceGate.releaseFirstCall() | ||
| await discardedSetup.wait() |
There was a problem hiding this comment.
These waits (also L62, L117 and L125) have no timeout, so if this regresses the test hangs instead of failing. Could you use an XCTestExpectation with await fulfillment(of:timeout:)?
| let baseRequest: URLRequest | ||
|
|
||
| // A narrow synchronization point for tests of task setup cancellation. | ||
| var setupDiscardedHandlerForTesting: (@Sendable () -> Void)? |
There was a problem hiding this comment.
This is an unsynchronized var on an @unchecked Sendable class, and it only fires at one of the three discard points. Could you guard it with stateLock (or #if DEBUG), and call it at every discard point?
This branch has not been deployed
2 participants
Summary
Testing
StorageTaskEnqueueTestsfor upload and download setup races.git diff --checkpassed.