diff --git a/FirebaseStorage/CHANGELOG.md b/FirebaseStorage/CHANGELOG.md index cdb5be246d1..078349e60a5 100644 --- a/FirebaseStorage/CHANGELOG.md +++ b/FirebaseStorage/CHANGELOG.md @@ -1,4 +1,6 @@ # 13.1.0 +- [fixed] Prevented duplicate upload and download requests when resuming a task before its initial + request setup has completed. (#16771) - [fixed] Fixed a race where an observer added while an upload or download task was completing could be called twice for the success or failure event. This could crash `putDataAsync`, `putFileAsync`, and `writeAsync` when called with an `onProgress` handler. (#16813) diff --git a/FirebaseStorage/Sources/Internal/StorageFetcherService.swift b/FirebaseStorage/Sources/Internal/StorageFetcherService.swift index 63af8e2fe2e..0bb0133e207 100644 --- a/FirebaseStorage/Sources/Internal/StorageFetcherService.swift +++ b/FirebaseStorage/Sources/Internal/StorageFetcherService.swift @@ -26,7 +26,11 @@ actor StorageFetcherService { private var _fetcherService: GTMSessionFetcherService? + // Allows unit tests to hold service setup across a task pause/resume transition. + private var serviceWaiterForTesting: (@Sendable () async -> Void)? + func service(_ storage: Storage) async -> GTMSessionFetcherService { + await serviceWaiterForTesting?() if let _fetcherService { return _fetcherService } @@ -77,6 +81,10 @@ actor StorageFetcherService { } } + func updateServiceWaiterForTesting(_ waiter: (@Sendable () async -> Void)?) { + serviceWaiterForTesting = waiter + } + private var testBlock: GTMSessionFetcherTestBlock? private var retryWhenOffline: GTMSessionFetcherRetryBlock = { diff --git a/FirebaseStorage/Sources/StorageDownloadTask.swift b/FirebaseStorage/Sources/StorageDownloadTask.swift index 474031f1f1d..460c1524224 100644 --- a/FirebaseStorage/Sources/StorageDownloadTask.swift +++ b/FirebaseStorage/Sources/StorageDownloadTask.swift @@ -38,8 +38,14 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u * Prepares a task and begins execution. */ @objc open func enqueue() { + let generation = stateLock.withLock { () -> UInt64? in + guard state == .unknown, fetcher == nil else { return nil } + enqueueGeneration &+= 1 + return enqueueGeneration + } + guard let generation else { return } Task { - await enqueueImplementation() + await enqueueImplementation(generation: generation) } } @@ -97,12 +103,14 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u @objc open func resume() { var downloadDataToResume: Data? var snapshotToFire: StorageTaskSnapshot? + var generation: UInt64 = 0 let shouldReturn1 = stateLock.withLock { () -> Bool in - guard state == .unknown || state == .queueing || state == .pausing || state == .paused || - state == .progress else { + guard state == .unknown || state == .queueing || state == .pausing || state == .paused else { return true } state = .resuming + enqueueGeneration &+= 1 + generation = enqueueGeneration downloadDataToResume = downloadData snapshotToFire = snapshotUnderLock() return false @@ -121,12 +129,17 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u if shouldEnqueue { Task { - await self.enqueueImplementation(resumeWith: downloadDataToResume) + await self.enqueueImplementation(resumeWith: downloadDataToResume, generation: generation) } } } private var fetcher: GTMSessionFetcher? + private var enqueueGeneration: UInt64 = 0 + var hasFetcherForTesting: Bool { + stateLock.withLock { fetcher != nil } + } + var downloadData: Data? // Hold completion in object to force it to be retained until completion block is called. var completionData: ((Data?, Error?) -> Void)? @@ -144,8 +157,10 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u self.fetcher?.stopFetching() } - private func enqueueImplementation(resumeWith resumeData: Data? = nil) async { + private func enqueueImplementation(resumeWith resumeData: Data? = nil, + generation: UInt64) async { let shouldProceed = stateLock.withLock { () -> Bool in + guard generation == enqueueGeneration else { return false } guard state == .unknown || state == .queueing || state == .resuming || state == .running || state == .progress else { return false @@ -153,7 +168,10 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u state = .queueing return true } - if !shouldProceed { return } + if !shouldProceed { + notifySetupDiscardedForTesting() + return + } var request = baseRequest request.httpMethod = "GET" @@ -168,6 +186,10 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u fetcher.comment = "Resuming DownloadTask" } else { let fetcherService = await StorageFetcherService.shared.service(reference.storage) + guard stateLock.withLock({ generation == enqueueGeneration }) else { + notifySetupDiscardedForTesting() + return + } fetcher = fetcherService.fetcher(with: request) fetcher.comment = "Starting DownloadTask" @@ -183,6 +205,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u guard let self = self else { return } var snapshotToFire: StorageTaskSnapshot? self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } guard self.state == .unknown || self.state == .queueing || self.state == .resuming || self.state == .running || self.state == .progress else { return @@ -197,6 +220,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u } self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } guard self.state == .unknown || self.state == .queueing || self.state == .resuming || self.state == .running || self.state == .progress else { return @@ -211,6 +235,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u guard let self = self, let fetcher = fetcher else { return } var snapshotToFire: StorageTaskSnapshot? self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } guard self.state == .unknown || self.state == .queueing || self.state == .resuming || self.state == .running || self.state == .progress else { return @@ -227,6 +252,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u } self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } guard self.state == .unknown || self.state == .queueing || self.state == .resuming || self.state == .running || self.state == .progress else { return @@ -237,6 +263,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u } var isPausing = false let shouldContinue = stateLock.withLock { () -> Bool in + guard generation == enqueueGeneration else { return false } if state == .cancelled || state == .pausing || state == .paused { isPausing = state == .pausing return false @@ -246,6 +273,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u return true } if !shouldContinue { + notifySetupDiscardedForTesting() if isPausing { fetcher.resumeDataBlock = { [weak self] (data: Data) in guard let self = self else { return } @@ -274,6 +302,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u var successSnapshot: StorageTaskSnapshot? stateLock.withLock { + guard generation == enqueueGeneration else { return } if state == .cancelled { return } state = .progress @@ -296,6 +325,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement, @u var failureSnapshot: StorageTaskSnapshot? stateLock.withLock { + guard generation == enqueueGeneration else { return } if state == .cancelled || state == .paused || state == .pausing { return } state = .progress diff --git a/FirebaseStorage/Sources/StorageTask.swift b/FirebaseStorage/Sources/StorageTask.swift index 75d0c0065d3..39c22c4815a 100644 --- a/FirebaseStorage/Sources/StorageTask.swift +++ b/FirebaseStorage/Sources/StorageTask.swift @@ -91,6 +91,18 @@ import Foundation let baseRequest: URLRequest + // A narrow synchronization point for tests of task setup cancellation. + private var setupDiscardedHandler: (@Sendable () -> Void)? + var setupDiscardedHandlerForTesting: (@Sendable () -> Void)? { + get { stateLock.withLock { setupDiscardedHandler } } + set { stateLock.withLock { setupDiscardedHandler = newValue } } + } + + func notifySetupDiscardedForTesting() { + let handler = stateLock.withLock { setupDiscardedHandler } + handler?() + } + init(reference: StorageReference, queue: DispatchQueue) { self.reference = reference diff --git a/FirebaseStorage/Sources/StorageUploadTask.swift b/FirebaseStorage/Sources/StorageUploadTask.swift index 49849ed6312..fbfc55151fb 100644 --- a/FirebaseStorage/Sources/StorageUploadTask.swift +++ b/FirebaseStorage/Sources/StorageUploadTask.swift @@ -44,11 +44,22 @@ import Foundation * Prepares a task and begins execution. */ @objc open func enqueue() { + let generation = stateLock.withLock { () -> UInt64? in + guard uploadFetcher == nil, + state == .unknown || state == .resuming || state == .running else { + return nil + } + enqueueGeneration &+= 1 + return enqueueGeneration + } + guard let generation else { return } + // Capturing self so that the upload is done whether or not there is a callback. dispatchQueue.async { [self] in let contentValidationError = self.contentUploadError() var failureSnapshot: StorageTaskSnapshot? let shouldProceed = stateLock.withLock { () -> Bool in + guard generation == enqueueGeneration else { return false } guard state == .unknown || state == .queueing || state == .resuming || state == .running || state == .progress else { return false @@ -65,6 +76,8 @@ import Foundation if !shouldProceed { if let failureSnapshot { self.finishTaskWithStatus(status: .failure, snapshot: failureSnapshot) + } else { + notifySetupDiscardedForTesting() } return } @@ -74,6 +87,10 @@ import Foundation Task { let fetcherService = await StorageFetcherService.shared.service(reference.storage) + guard stateLock.withLock({ generation == enqueueGeneration }) else { + notifySetupDiscardedForTesting() + return + } var request = self.baseRequest request.httpMethod = "POST" request.timeoutInterval = self.reference.storage.maxUploadRetryTime @@ -127,6 +144,7 @@ import Foundation guard let self = self else { return } var snapshotToFire: StorageTaskSnapshot? self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } guard self.state == .unknown || self.state == .queueing || self.state == .resuming || self.state == .running || self.state == .progress else { return @@ -142,6 +160,7 @@ import Foundation } self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } guard self.state == .unknown || self.state == .queueing || self.state == .resuming || self.state == .running || self.state == .progress else { return @@ -152,6 +171,7 @@ import Foundation // Process fetches var isPaused = false let shouldContinue = self.stateLock.withLock { () -> Bool in + guard generation == self.enqueueGeneration else { return false } if self.state == .cancelled || self.state == .pausing || self.state == .paused { isPaused = self.state == .paused || self.state == .pausing return false @@ -161,6 +181,7 @@ import Foundation return true } if !shouldContinue { + notifySetupDiscardedForTesting() if isPaused { uploadFetcher.pauseFetching() } else { @@ -175,6 +196,7 @@ import Foundation var failureSnapshot: StorageTaskSnapshot? self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } if self.state == .cancelled { return } self.state = .progress @@ -207,6 +229,7 @@ import Foundation var failureSnapshot: StorageTaskSnapshot? self.stateLock.withLock { + guard generation == self.enqueueGeneration else { return } if self.state == .cancelled || self.state == .paused || self .state == .pausing { return } @@ -320,6 +343,11 @@ import Foundation } private var uploadFetcher: GTMSessionUploadFetcher? + private var enqueueGeneration: UInt64 = 0 + var hasFetcherForTesting: Bool { + stateLock.withLock { uploadFetcher != nil } + } + private var uploadMetadata: StorageMetadata private var uploadData: Data? // Hold completion in object to force it to be retained until completion block is called. diff --git a/FirebaseStorage/Tests/Unit/StorageTaskEnqueueTests.swift b/FirebaseStorage/Tests/Unit/StorageTaskEnqueueTests.swift new file mode 100644 index 00000000000..51accdc942b --- /dev/null +++ b/FirebaseStorage/Tests/Unit/StorageTaskEnqueueTests.swift @@ -0,0 +1,268 @@ +// Copyright 2026 Google LLC +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +@testable import FirebaseStorage +import Foundation +#if COCOAPODS + import GTMSessionFetcher +#else + import GTMSessionFetcherCore +#endif +import XCTest + +class StorageTaskEnqueueTests: StorageTestHelpers { + func testDownloadResumeDuringProgressDoesNotStartAnotherRequest() async throws { + let firstCallEntered = expectation(description: "Download setup enters service") + let serviceGate = FirstCallServiceGate(firstCallEntered: firstCallEntered) + let responseLatch = FetcherResponseLatch() + let requestCount = FetcherRequestCounter() + let progressCount = FetcherRequestCounter() + let progressObserved = expectation(description: "Resume from download progress") + let completed = expectation(description: "Download completes") + let testBlock: GTMSessionFetcherTestBlock = { fetcher, response in + requestCount.increment() + guard let url = fetcher.request?.url else { + XCTFail("Fetcher request should have a URL") + return + } + responseLatch.install { + response( + HTTPURLResponse(url: url, statusCode: 200, httpVersion: "HTTP/1.1", headerFields: nil), + Data("download".utf8), + nil + ) + } + guard let receivedProgress = fetcher.receivedProgressBlock else { + XCTFail("Download should install its received-progress callback") + responseLatch.release() + return + } + receivedProgress(1, 1) + } + await StorageFetcherService.shared.updateTestBlock(testBlock) + await StorageFetcherService.shared.updateServiceWaiterForTesting { + await serviceGate.waitForFirstCall() + } + addTeardownBlock { + responseLatch.release() + await serviceGate.releaseFirstCall() + await StorageFetcherService.shared.updateTestBlock(nil) + await StorageFetcherService.shared.updateServiceWaiterForTesting(nil) + } + let task = rootReference().child("object").getData(maxSize: 1024) { data, error in + XCTAssertNil(error) + XCTAssertEqual(data, Data("download".utf8)) + completed.fulfill() + } + task.observe(.progress) { _ in + guard progressCount.increment() == 1 else { return } + // Make the transient progress phase deterministic before completion is released. + task.stateLock.withLock { task.state = .progress } + task.resume() + XCTAssertTrue(task.hasFetcherForTesting) + progressObserved.fulfill() + responseLatch.release() + } + await fulfillment(of: [firstCallEntered], timeout: 10) + await serviceGate.releaseFirstCall() + await fulfillment(of: [progressObserved, completed], timeout: 10) + XCTAssertEqual(requestCount.value, 1) + } + + func testUploadResumeDiscardsEarlierSetup() async throws { + try await uploadResumeDiscardsEarlierSetup(pauseFirst: true) + } + + func testImmediateUploadResumeDiscardsEarlierSetup() async throws { + try await uploadResumeDiscardsEarlierSetup(pauseFirst: false) + } + + private func uploadResumeDiscardsEarlierSetup(pauseFirst: Bool) async throws { + let firstCallEntered = expectation(description: "Initial upload setup enters service") + let serviceGate = FirstCallServiceGate(firstCallEntered: firstCallEntered) + let discardedSetup = expectation(description: "Initial upload setup discarded") + let requestCount = FetcherRequestCounter() + let requestStarted = expectation(description: "Resumed upload starts one request") + let testBlock: GTMSessionFetcherTestBlock = { fetcher, response in + if requestCount.increment() == 1 { + requestStarted.fulfill() + } + guard let responseURL = fetcher.request?.url else { + XCTFail("Fetcher request should have a URL") + return + } + response( + HTTPURLResponse( + url: responseURL, + statusCode: 200, + httpVersion: "HTTP/1.1", + headerFields: nil + ), + Data("{}".utf8), + nil + ) + } + await StorageFetcherService.shared.updateTestBlock(testBlock) + await StorageFetcherService.shared.updateServiceWaiterForTesting { + await serviceGate.waitForFirstCall() + } + addTeardownBlock { + await serviceGate.releaseFirstCall() + await StorageFetcherService.shared.updateTestBlock(nil) + await StorageFetcherService.shared.updateServiceWaiterForTesting(nil) + } + + let task = rootReference().child("object").putData(Data("upload".utf8)) + task.setupDiscardedHandlerForTesting = { + discardedSetup.fulfill() + } + await fulfillment(of: [firstCallEntered], timeout: 10) + + if pauseFirst { task.pause() } + task.resume() + await fulfillment(of: [requestStarted], timeout: 10) + XCTAssertTrue(task.hasFetcherForTesting) + + await serviceGate.releaseFirstCall() + await fulfillment(of: [discardedSetup], timeout: 10) + XCTAssertEqual(requestCount.value, 1) + } + + func testDownloadResumeDiscardsEarlierSetup() async throws { + try await downloadResumeDiscardsEarlierSetup(pauseFirst: true) + } + + func testImmediateDownloadResumeDiscardsEarlierSetup() async throws { + try await downloadResumeDiscardsEarlierSetup(pauseFirst: false) + } + + private func downloadResumeDiscardsEarlierSetup(pauseFirst: Bool) async throws { + let firstCallEntered = expectation(description: "Initial download setup enters service") + let serviceGate = FirstCallServiceGate(firstCallEntered: firstCallEntered) + let discardedSetup = expectation(description: "Initial download setup discarded") + let requestCount = FetcherRequestCounter() + let requestStarted = expectation(description: "Resumed download starts one request") + let testBlock: GTMSessionFetcherTestBlock = { fetcher, response in + if requestCount.increment() == 1 { + requestStarted.fulfill() + } + guard let responseURL = fetcher.request?.url else { + XCTFail("Fetcher request should have a URL") + return + } + response( + HTTPURLResponse( + url: responseURL, + statusCode: 200, + httpVersion: "HTTP/1.1", + headerFields: nil + ), + Data("download".utf8), + nil + ) + } + await StorageFetcherService.shared.updateTestBlock(testBlock) + await StorageFetcherService.shared.updateServiceWaiterForTesting { + await serviceGate.waitForFirstCall() + } + addTeardownBlock { + await serviceGate.releaseFirstCall() + await StorageFetcherService.shared.updateTestBlock(nil) + await StorageFetcherService.shared.updateServiceWaiterForTesting(nil) + } + + let task = rootReference().child("object").getData(maxSize: 1024) { _, _ in } + task.setupDiscardedHandlerForTesting = { + discardedSetup.fulfill() + } + await fulfillment(of: [firstCallEntered], timeout: 10) + + if pauseFirst { task.pause() } + task.resume() + await fulfillment(of: [requestStarted], timeout: 10) + XCTAssertTrue(task.hasFetcherForTesting) + + await serviceGate.releaseFirstCall() + await fulfillment(of: [discardedSetup], timeout: 10) + XCTAssertEqual(requestCount.value, 1) + } +} + +private actor FirstCallServiceGate { + let firstCallEntered: XCTestExpectation + private var callCount = 0 + private var isReleased = false + private var releaseContinuation: CheckedContinuation? + + init(firstCallEntered: XCTestExpectation) { + self.firstCallEntered = firstCallEntered + } + + func waitForFirstCall() async { + callCount += 1 + guard callCount == 1, !isReleased else { return } + await withCheckedContinuation { continuation in + releaseContinuation = continuation + firstCallEntered.fulfill() + } + } + + func releaseFirstCall() { + isReleased = true + releaseContinuation?.resume() + releaseContinuation = nil + } +} + +private final class FetcherResponseLatch: @unchecked Sendable { + private let lock = NSLock() + private var response: (() -> Void)? + private var isReleased = false + + func install(_ response: @escaping () -> Void) { + let shouldRun = lock.withLock { + if isReleased { return true } + self.response = response + return false + } + if shouldRun { response() } + } + + func release() { + let response = lock.withLock { + isReleased = true + let response = self.response + self.response = nil + return response + } + response?() + } +} + +private final class FetcherRequestCounter: @unchecked Sendable { + private let lock = NSLock() + private var count = 0 + + @discardableResult + func increment() -> Int { + lock.withLock { + count += 1 + return count + } + } + + var value: Int { + lock.withLock { count } + } +}