Skip to content

Commit 9b5be20

Browse files
paulb777paulb777
authored
[Storage] Fix fetcherServiceMap potential race (#13446)
1 parent 27d462b commit 9b5be20

11 files changed

+164
-308
lines changed

‎FirebaseStorage/Sources/Internal/StorageFetcherService.swift

Lines changed: 31 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -23,44 +23,43 @@ import Foundation
2323
/// Manage Storage's fetcherService
2424
@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *)
2525
actor StorageFetcherService {
26+
static let shared = StorageFetcherService()
27+
2628
private var _fetcherService: GTMSessionFetcherService?
2729

28-
func fetcherService(_ storage: Storage) -> GTMSessionFetcherService {
30+
func service(_ storage: Storage) async -> GTMSessionFetcherService {
2931
if let _fetcherService {
3032
return _fetcherService
3133
}
3234
let app = storage.app
33-
if StorageFetcherService.fetcherServiceMap[app.name] == nil {
34-
StorageFetcherService.fetcherServiceMap[app.name] = [:]
35-
}
36-
var fetcherService = StorageFetcherService.fetcherServiceMap[app.name]?[storage.storageBucket]
37-
if fetcherService == nil {
38-
fetcherService = GTMSessionFetcherService()
39-
fetcherService?.isRetryEnabled = true
40-
fetcherService?.retryBlock = retryWhenOffline
41-
fetcherService?.allowLocalhostRequest = true
42-
fetcherService?.maxRetryInterval = storage.maxOperationRetryInterval
43-
fetcherService?.testBlock = testBlock
35+
if let fetcherService = getFromMap(appName: app.name, bucket: storage.storageBucket) {
36+
return fetcherService
37+
} else {
38+
let fetcherService = GTMSessionFetcherService()
39+
fetcherService.isRetryEnabled = true
40+
fetcherService.retryBlock = retryWhenOffline
41+
fetcherService.allowLocalhostRequest = true
42+
fetcherService.maxRetryInterval = storage.maxOperationRetryInterval
43+
fetcherService.testBlock = testBlock
4444
let authorizer = StorageTokenAuthorizer(
4545
googleAppID: app.options.googleAppID,
4646
callbackQueue: storage.callbackQueue,
4747
authProvider: storage.auth,
4848
appCheck: storage.appCheck
4949
)
50-
fetcherService?.authorizer = authorizer
51-
StorageFetcherService.fetcherServiceMap[app.name]?[storage.storageBucket] = fetcherService
52-
}
53-
if storage.usesEmulator {
54-
fetcherService?.allowLocalhostRequest = true
55-
fetcherService?.allowedInsecureSchemes = ["http"]
50+
fetcherService.authorizer = authorizer
51+
if storage.usesEmulator {
52+
fetcherService.allowLocalhostRequest = true
53+
fetcherService.allowedInsecureSchemes = ["http"]
54+
}
55+
setMap(appName: app.name, bucket: storage.storageBucket, fetcher: fetcherService)
56+
return fetcherService
5657
}
57-
_fetcherService = fetcherService
58-
return fetcherService!
5958
}
6059

6160
/// Update the testBlock for unit testing. Save it as a property since this may be called before
6261
/// fetcherService is initialized.
63-
func updateTestBlock(_ block: @escaping GTMSessionFetcherTestBlock) {
62+
func updateTestBlock(_ block: GTMSessionFetcherTestBlock?) {
6463
testBlock = block
6564
if let _fetcherService {
6665
_fetcherService.testBlock = testBlock
@@ -69,9 +68,6 @@ actor StorageFetcherService {
6968

7069
private var testBlock: GTMSessionFetcherTestBlock?
7170

72-
/// Map of apps to a dictionary of buckets to GTMSessionFetcherService.
73-
private static var fetcherServiceMap: [String: [String: GTMSessionFetcherService]] = [:]
74-
7571
private var retryWhenOffline: GTMSessionFetcherRetryBlock = {
7672
(suggestedWillRetry: Bool,
7773
error: Error?,
@@ -84,4 +80,15 @@ actor StorageFetcherService {
8480
}
8581
response(shouldRetry)
8682
}
83+
84+
/// Map of apps to a dictionary of buckets to GTMSessionFetcherService.
85+
private var fetcherServiceMap: [String: [String: GTMSessionFetcherService]] = [:]
86+
87+
private func getFromMap(appName: String, bucket: String) -> GTMSessionFetcherService? {
88+
return fetcherServiceMap[appName]?[bucket]
89+
}
90+
91+
private func setMap(appName: String, bucket: String, fetcher: GTMSessionFetcherService) {
92+
fetcherServiceMap[appName, default: [:]][bucket] = fetcher
93+
}
8794
}

‎FirebaseStorage/Sources/Internal/StorageInternalTask.swift

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -38,9 +38,7 @@ class StorageInternalTask: StorageTask {
3838
dispatchQueue.async { [self] in
3939
self.state = .queueing
4040
Task {
41-
let fetcherService = await reference.storage.fetcherService
42-
.fetcherService(reference.storage)
43-
41+
let fetcherService = await StorageFetcherService.shared.service(reference.storage)
4442
var request = request ?? self.baseRequest
4543
request.httpMethod = httpMethod
4644
request.timeoutInterval = self.reference.storage.maxOperationRetryTime

‎FirebaseStorage/Sources/Storage.swift

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,6 @@ import FirebaseCore
266266
}
267267
}
268268

269-
let fetcherService = StorageFetcherService()
270-
271269
let dispatchQueue: DispatchQueue
272270

273271
init(app: FirebaseApp, bucket: String) {

‎FirebaseStorage/Sources/StorageDownloadTask.swift

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ open class StorageDownloadTask: StorageObservableTask, StorageTaskManagement {
121121
fetcher = GTMSessionFetcher(downloadResumeData: resumeData)
122122
fetcher.comment = "Resuming DownloadTask"
123123
} else {
124-
let fetcherService = await reference.storage.fetcherService.fetcherService(reference.storage)
124+
let fetcherService = await StorageFetcherService.shared.service(reference.storage)
125125

126126
fetcher = fetcherService.fetcher(with: request)
127127
fetcher.comment = "Starting DownloadTask"

‎FirebaseStorage/Sources/StorageUploadTask.swift

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -59,8 +59,7 @@ import Foundation
5959
let bodyData = try? JSONSerialization.data(withJSONObject: dataRepresentation)
6060

6161
Task {
62-
let fetcherService = await reference.storage.fetcherService
63-
.fetcherService(reference.storage)
62+
let fetcherService = await StorageFetcherService.shared.service(reference.storage)
6463
var request = self.baseRequest
6564
request.httpMethod = "POST"
6665
request.timeoutInterval = self.reference.storage.maxUploadRetryTime

‎FirebaseStorage/Tests/Unit/StorageAuthorizerTests.swift

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ class StorageAuthorizerTests: StorageTestHelpers {
2323
var appCheckTokenSuccess: FIRAppCheckTokenResultFake!
2424
var appCheckTokenError: FIRAppCheckTokenResultFake!
2525
var fetcher: GTMSessionFetcher!
26-
var fetcherService: GTMSessionFetcherService!
2726
var auth: FIRAuthInteropFake!
2827
var appCheck: FIRAppCheckFake!
2928

@@ -52,7 +51,6 @@ class StorageAuthorizerTests: StorageTestHelpers {
5251

5352
override func tearDown() {
5453
fetcher = nil
55-
fetcherService = nil
5654
auth = nil
5755
appCheck = nil
5856
appCheckTokenSuccess = nil

‎FirebaseStorage/Tests/Unit/StorageDeleteTests.swift

Lines changed: 26 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -19,29 +19,9 @@ import XCTest
1919

2020
@available(iOS 13, tvOS 13, macOS 10.15, macCatalyst 13, watchOS 7, *)
2121
class StorageDeleteTests: StorageTestHelpers {
22-
var fetcherService: GTMSessionFetcherService?
23-
var dispatchQueue: DispatchQueue?
24-
25-
override func setUp() {
26-
super.setUp()
27-
fetcherService = GTMSessionFetcherService()
28-
fetcherService?.authorizer = StorageTokenAuthorizer(
29-
googleAppID: "dummyAppID",
30-
authProvider: nil,
31-
appCheck: nil
32-
)
33-
dispatchQueue = DispatchQueue(label: "Test dispatch queue")
34-
}
35-
36-
override func tearDown() {
37-
fetcherService = nil
38-
super.tearDown()
39-
}
40-
41-
func testFetcherConfiguration() {
42-
let expectation = self.expectation(description: #function)
43-
fetcherService!.testBlock = { (fetcher: GTMSessionFetcher!,
44-
response: GTMSessionFetcherTestResponse) in
22+
func testFetcherConfiguration() async {
23+
let testBlock = { (fetcher: GTMSessionFetcher!,
24+
response: GTMSessionFetcherTestResponse) in
4525
XCTAssertEqual(fetcher.request?.url, self.objectURL())
4626
XCTAssertEqual(fetcher.request?.httpMethod, "DELETE")
4727
let httpResponse = HTTPURLResponse(
@@ -52,67 +32,46 @@ class StorageDeleteTests: StorageTestHelpers {
5232
)
5333
response(httpResponse, nil, nil)
5434
}
35+
await StorageFetcherService.shared.updateTestBlock(testBlock)
5536
let path = objectPath()
5637
let ref = StorageReference(storage: storage(), path: path)
57-
StorageDeleteTask.deleteTask(
58-
reference: ref,
59-
queue: dispatchQueue!.self
60-
) { _, error in
61-
expectation.fulfill()
38+
do {
39+
let _ = try await ref.delete()
40+
} catch {
41+
// All testing is in test block.
6242
}
63-
waitForExpectation(test: self)
6443
}
6544

66-
func testSuccessfulFetch() {
67-
let expectation = self.expectation(description: #function)
68-
fetcherService!.testBlock = { (fetcher: GTMSessionFetcher!,
69-
response: GTMSessionFetcherTestResponse) in
70-
XCTAssertEqual(fetcher.request?.url, self.objectURL())
71-
XCTAssertEqual(fetcher.request?.httpMethod, "DELETE")
72-
let httpResponse = HTTPURLResponse(
73-
url: (fetcher.request?.url)!,
74-
statusCode: 200,
75-
httpVersion: "HTTP/1.1",
76-
headerFields: nil
77-
)
78-
response(httpResponse, nil, nil)
79-
}
45+
func testSuccessfulFetch() async {
46+
await StorageFetcherService.shared.updateTestBlock(successBlock())
8047
let path = objectPath()
8148
let ref = StorageReference(storage: storage(), path: path)
82-
StorageDeleteTask.deleteTask(
83-
reference: ref,
84-
queue: dispatchQueue!.self
85-
) { _, error in
86-
expectation.fulfill()
49+
do {
50+
let _ = try await ref.delete()
51+
} catch {
52+
// All testing is in test block.
8753
}
88-
waitForExpectation(test: self)
8954
}
9055

91-
func testSuccessfulFetchWithEmulator() {
92-
let expectation = self.expectation(description: #function)
56+
func testSuccessfulFetchWithEmulator() async {
9357
let storage = self.storage()
9458
storage.useEmulator(withHost: "localhost", port: 8080)
95-
fetcherService?.allowLocalhostRequest = true
96-
97-
fetcherService!
98-
.testBlock = successBlock(
99-
withURL: URL(string: "http://localhost:8080/v0/b/bucket/o/object")!
100-
)
101-
59+
let testBlock = successBlock(
60+
withURL: URL(string: "http://localhost:8080/v0/b/bucket/o/object")!
61+
)
62+
await StorageFetcherService.shared.updateTestBlock(successBlock())
10263
let path = objectPath()
10364
let ref = StorageReference(storage: storage, path: path)
104-
StorageDeleteTask.deleteTask(
105-
reference: ref,
106-
queue: dispatchQueue!.self
107-
) { _, error in
108-
expectation.fulfill()
65+
do {
66+
let _ = try await ref.delete()
67+
} catch {
68+
// All testing is in test block.
10969
}
110-
waitForExpectation(test: self)
11170
}
11271

11372
func testUnsuccessfulFetchUnauthenticated() async {
11473
let storage = storage()
115-
await storage.fetcherService.updateTestBlock(unauthenticatedBlock())
74+
await StorageFetcherService.shared.updateTestBlock(unauthenticatedBlock())
11675
let path = objectPath()
11776
let ref = StorageReference(storage: storage, path: path)
11877
do {
@@ -124,7 +83,7 @@ class StorageDeleteTests: StorageTestHelpers {
12483

12584
func testUnsuccessfulFetchUnauthorized() async {
12685
let storage = storage()
127-
await storage.fetcherService.updateTestBlock(unauthorizedBlock())
86+
await StorageFetcherService.shared.updateTestBlock(unauthorizedBlock())
12887
let path = objectPath()
12988
let ref = StorageReference(storage: storage, path: path)
13089
do {
@@ -136,7 +95,7 @@ class StorageDeleteTests: StorageTestHelpers {
13695

13796
func testUnsuccessfulFetchObjectDoesntExist() async {
13897
let storage = storage()
139-
await storage.fetcherService.updateTestBlock(notFoundBlock())
98+
await StorageFetcherService.shared.updateTestBlock(notFoundBlock())
14099
let path = objectPath()
141100
let ref = StorageReference(storage: storage, path: path)
142101
do {

0 commit comments

Comments
 (0)