Fix StorageUploadTask cancel - #16359

Code Review

This pull request aims to resolve race conditions in Storage task cancellation and improve Swift 6 strict concurrency compliance by introducing NSLock (stateLock) and adopting @unchecked Sendable across various StorageTask subclasses. While these changes are a step in the right direction, the code review highlights several critical issues that must be addressed. Specifically, calling fetcher control methods (such as stopFetching, pauseFetching, and resumeFetching) while holding the stateLock poses a high risk of deadlocks. Additionally, there are multiple instances of data races where shared properties (like fetcher, uploadFetcher, downloadData, and state maps) are accessed or mutated outside of the lock. Finally, some synchronization can be simplified, such as removing redundant locking when iterating over local copies of handler dictionaries.

Code Review

This pull request addresses race conditions in Firebase Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing NSLock synchronization and marking tasks as @unchecked Sendable. However, the review highlights several critical concurrency issues: a return statement inside a lock closure in resume() fails to prevent task enqueuing; race conditions exist when pausing or cancelling tasks in the .queueing state; paused tasks can trigger spurious failure events because the catch blocks do not check for paused states; and potential data races remain due to unsynchronized reads of handlerDictionaries.

Code Review

This pull request addresses a race condition in Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing a stateLock (NSLock) to protect mutable task state and marking tasks as @unchecked Sendable. The review feedback highlights several remaining data races where properties like state, fetcher, and uploadFetcher are accessed outside of the lock in StorageObservableTask, StorageDownloadTask, and StorageUploadTask. The reviewer suggests capturing these properties locally within the lock or using existing local variables to ensure complete thread safety.

Code Review

This pull request addresses race conditions in Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing stateLock to protect mutable state across various StorageTask subclasses, which are now marked as @unchecked Sendable. It also adds integration tests for upload cancellation. The review feedback highlights two remaining data races where state in StorageObservableTask and fetcher in StorageDownloadTask are accessed outside of the stateLock, and provides actionable suggestions to resolve them.

Code Review

This pull request addresses race conditions in Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing a stateLock (NSLock) and marking tasks as @unchecked Sendable. The review feedback highlights several critical state-management issues where task states (such as .cancelled, .paused, .success, or .failed) could be incorrectly overwritten or where duplicate observer events could be fired. Specifically, registering a new observer might re-trigger all existing observers, and asynchronous task execution or late-dispatched progress/resume blocks could resurrect or overwrite terminal states.

Code Review

This pull request addresses race conditions in Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing stateLock (NSLock) and @unchecked Sendable across various StorageTask classes. The review feedback highlights several critical race conditions and state-corruption risks in the new locking logic. Specifically, late-firing progress or pause callbacks in StorageDownloadTask and StorageUploadTask could erroneously revert completed or failed tasks back to .running or .paused states. Additionally, a race condition in StorageObservableTask between capturing a snapshot and registering an observer could cause observers to miss status notifications. The reviewer recommends atomic observer registration and stricter state validation before state transitions.

Code Review

This pull request improves Swift 6 strict concurrency compliance and thread safety across Firebase Storage tasks by replacing objc_sync_enter/exit with an explicit NSLock (stateLock) and marking tasks as @unchecked Sendable. While these changes resolve several race conditions, the review highlights critical issues in StorageUploadTask and StorageInternalTask. Specifically, StorageUploadTask.enqueue() can unconditionally overwrite a cancelled or paused state, and its resume() method fails to recover if the task was paused before the fetcher was initialized. Additionally, a potential data race remains in StorageInternalTask where the fetcher property is accessed concurrently without synchronization.

Code Review

This pull request addresses a race condition in Storage task cancellation and improves Swift 6 strict concurrency compliance by replacing Objective-C synchronization with a dedicated NSLock (stateLock) and marking storage tasks as @unchecked Sendable. The reviewer provided valuable feedback pointing out three issues: in StorageDownloadTask, the final progress updates in both the success and error paths are fired with incorrect statuses because the state is not temporarily transitioned to .progress under the lock; and in StorageUploadTask, calling contentUploadError() inside the lock performs synchronous file I/O and could block other threads, so it should be moved outside the lock.

Code Review

This pull request addresses race conditions in Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing an NSLock (stateLock) to protect mutable state across various StorageTask subclasses. It also marks these tasks as @unchecked Sendable and replaces Objective-C synchronization blocks with lock-based synchronization. Additionally, integration tests for cancelling uploads have been added. The review feedback suggests optimizing lock usage in both StorageDownloadTask and StorageUploadTask by capturing the task's paused/pausing state within the initial lock block, which avoids unnecessary lock re-acquisition and ensures better atomicity.

Code Review

This pull request addresses race conditions in Storage task cancellation and improves Swift 6 strict concurrency compliance by introducing stateLock (NSLock) to protect mutable state across StorageTask subclasses, marking them as @unchecked Sendable. It also adds integration tests for upload cancellation. The review feedback highlights two critical issues: first, calling pause() on StorageDownloadTask before the fetcher is initialized can cause the task to get stuck in the .pausing state; second, a JSON parsing failure in StorageUploadTask incorrectly transitions the task to a .success state despite setting an error. Both issues should be resolved using the provided code suggestions.

Code Review

This pull request addresses a race condition in Storage task cancellation and improves Swift 6 strict concurrency compliance for StorageTask by introducing a stateLock (NSLock) to protect mutable state across various tasks (StorageInternalTask, StorageDownloadTask, StorageUploadTask, StorageObservableTask). It marks these classes as @unchecked Sendable and updates state transitions and observer handling to be thread-safe. Additionally, integration tests are added to verify task cancellation during uploads. The review feedback suggests capturing the local fetcher weakly in receivedProgressBlock to avoid a potential retain cycle and unnecessary synchronized reads, and recommends using the more idiomatic UUID().uuidString instead of NSUUID().uuidString in Swift.

Code Review

This pull request addresses a race condition in Storage task cancellation and improves Swift 6 strict concurrency compliance for StorageTask. It replaces objc_sync_enter/objc_sync_exit with a dedicated NSLock (stateLock) across StorageTask and its subclasses (StorageInternalTask, StorageDownloadTask, StorageUploadTask, and StorageObservableTask) to safely synchronize access to mutable state, progress, and observers. Additionally, these classes are marked as @unchecked Sendable. Integration tests have been added to verify upload cancellation in both Swift and Objective-C. There are no review comments, and I have no feedback to provide.

LGTM with an optional suggestion.