Skip to content

Commit f534e15

Browse files
sumeruchatclaudejoaodordio
authored
SDK-471 Fix Swift access race in Pending/Fulfill (#1063)
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Joao Dordio <joaodordio@icloud.com>
1 parent be4a5a3 commit f534e15

3 files changed

Lines changed: 418 additions & 53 deletions

File tree

swift-sdk.xcodeproj/project.pbxproj

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,6 +405,7 @@
405405
ACE34AB5213776CB00691224 /* LocalStorageTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ACE34AB4213776CB00691224 /* LocalStorageTests.swift */; };
406406
ACED4C01213F50B30055A497 /* LoggingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ACED4C00213F50B30055A497 /* LoggingTests.swift */; };
407407
ACEDF41F2183C436000B9BFE /* PendingTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ACEDF41E2183C436000B9BFE /* PendingTests.swift */; };
408+
5C0471F221F0F0F0000B7CCC /* PendingConcurrencyTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 5C0471F121F0F0F0000B7CCC /* PendingConcurrencyTests.swift */; };
408409
ACF406252507F90F005FD775 /* NetworkConnectivityManagerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = ACF406242507F90F005FD775 /* NetworkConnectivityManagerTests.swift */; };
409410
ACF560D620E443BF000AAC23 /* AppDelegate.swift in Sources */ = {isa = PBXBuildFile; fileRef = ACF560D520E443BF000AAC23 /* AppDelegate.swift */; };
410411
ACF560DB20E443BF000AAC23 /* Main.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = ACF560D920E443BF000AAC23 /* Main.storyboard */; };
@@ -850,6 +851,7 @@
850851
ACE34AB4213776CB00691224 /* LocalStorageTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LocalStorageTests.swift; sourceTree = "<group>"; };
851852
ACED4C00213F50B30055A497 /* LoggingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LoggingTests.swift; sourceTree = "<group>"; };
852853
ACEDF41E2183C436000B9BFE /* PendingTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PendingTests.swift; sourceTree = "<group>"; };
854+
5C0471F121F0F0F0000B7CCC /* PendingConcurrencyTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = PendingConcurrencyTests.swift; sourceTree = "<group>"; };
853855
ACF406242507F90F005FD775 /* NetworkConnectivityManagerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NetworkConnectivityManagerTests.swift; sourceTree = "<group>"; };
854856
ACF560D320E443BF000AAC23 /* host-app.app */ = {isa = PBXFileReference; explicitFileType = wrapper.application; includeInIndex = 0; path = "host-app.app"; sourceTree = BUILT_PRODUCTS_DIR; };
855857
ACF560D520E443BF000AAC23 /* AppDelegate.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = AppDelegate.swift; sourceTree = "<group>"; };
@@ -1373,6 +1375,7 @@
13731375
ACED4C00213F50B30055A497 /* LoggingTests.swift */,
13741376
55B37FC5229752DD0042F13A /* OrderedDictionaryTests.swift */,
13751377
ACEDF41E2183C436000B9BFE /* PendingTests.swift */,
1378+
5C0471F121F0F0F0000B7CCC /* PendingConcurrencyTests.swift */,
13761379
);
13771380
name = "foundational-tests";
13781381
sourceTree = "<group>";
@@ -2461,6 +2464,7 @@
24612464
5588DFA128C04570000697D7 /* MockApplicationStateProvider.swift in Sources */,
24622465
5588DFF128C046FF000697D7 /* MockMessageViewControllerEventTracker.swift in Sources */,
24632466
ACEDF41F2183C436000B9BFE /* PendingTests.swift in Sources */,
2467+
5C0471F221F0F0F0000B7CCC /* PendingConcurrencyTests.swift in Sources */,
24642468
AC1B29002742579000AD2BE3 /* InAppNavigationTests.swift in Sources */,
24652469
5588DF7128C0442D000697D7 /* MockDateProvider.swift in Sources */,
24662470
1CBFFE1A2A97AEEF00ED57EE /* EmbeddedManagerTests.swift in Sources */,

swift-sdk/Internal/Pending.swift

Lines changed: 71 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -21,78 +21,98 @@ extension IterableError: LocalizedError {
2121
// either there is a success with result
2222
// or there is a failure with error
2323
// There is no way to set value a result in this class.
24+
//
25+
// Thread-safety: all mutations of `result`, `successCallbacks`, and `errorCallbacks`
26+
// are serialized through `stateQueue`. Callbacks are snapshotted under `.sync` and
27+
// invoked outside the critical section, so user code that re-enters `onSuccess` /
28+
// `onError` on the same instance cannot deadlock the queue.
2429
public class Pending<Value, Failure> where Failure: Error {
2530
fileprivate var successCallbacks = [(Value) -> Void]()
2631
fileprivate var errorCallbacks = [(Failure) -> Void]()
27-
32+
fileprivate var result: Result<Value, Failure>?
33+
34+
private let stateQueue = DispatchQueue(label: "com.iterable.pending.state")
35+
2836
public func onCompletion(receiveValue: @escaping ((Value) -> Void), receiveError: ( (Failure) -> Void)? = nil) {
29-
successCallbacks.append(receiveValue)
30-
31-
// if a successful result already exists (from constructor), report it
32-
if case let Result.success(value)? = result {
33-
successCallbacks.forEach { $0(value) }
34-
}
35-
36-
if let receiveError = receiveError {
37-
errorCallbacks.append(receiveError)
38-
39-
// if a failed result already exists (from constructor), report it
40-
if case let Result.failure(error)? = result {
41-
errorCallbacks.forEach { $0(error) }
37+
// Append both callbacks in one critical section so a concurrent failure
38+
// cannot land between the success and error registrations.
39+
let current: Result<Value, Failure>? = stateQueue.sync {
40+
successCallbacks.append(receiveValue)
41+
if let receiveError = receiveError {
42+
errorCallbacks.append(receiveError)
4243
}
44+
return result
45+
}
46+
47+
// Late registration on an already-resolved Pending: replay the current
48+
// result for the newly registered callback only.
49+
guard let current = current else { return }
50+
switch current {
51+
case let .success(value):
52+
receiveValue(value)
53+
case let .failure(error):
54+
receiveError?(error)
4355
}
4456
}
45-
57+
4658
@discardableResult public func onSuccess(block: @escaping ((Value) -> Void)) -> Pending<Value, Failure> {
47-
successCallbacks.append(block)
48-
49-
// if a successful result already exists (from constructor), report it
50-
if case let Result.success(value)? = result {
51-
successCallbacks.forEach { $0(value) }
59+
let current: Result<Value, Failure>? = stateQueue.sync {
60+
successCallbacks.append(block)
61+
return result
62+
}
63+
64+
if case let .success(value)? = current {
65+
block(value)
5266
}
53-
5467
return self
5568
}
56-
69+
5770
@discardableResult public func onError(block: @escaping ((Failure) -> Void)) -> Pending<Value, Failure> {
58-
errorCallbacks.append(block)
59-
60-
// if a failed result already exists (from constructor), report it
61-
if case let Result.failure(error)? = result {
62-
errorCallbacks.forEach { $0(error) }
71+
let current: Result<Value, Failure>? = stateQueue.sync {
72+
errorCallbacks.append(block)
73+
return result
74+
}
75+
76+
if case let .failure(error)? = current {
77+
block(error)
6378
}
64-
6579
return self
6680
}
67-
81+
6882
public func isResolved() -> Bool {
69-
result != nil
83+
stateQueue.sync { result != nil }
7084
}
71-
85+
7286
public func wait() {
7387
ITBDebug()
7488
guard !isResolved() else {
7589
ITBDebug("isResolved")
7690
return
7791
}
78-
92+
7993
ITBDebug("waiting....")
8094
Thread.sleep(forTimeInterval: 0.1)
8195
wait()
8296
}
83-
84-
fileprivate var result: Result<Value, Failure>? {
85-
// Observe whenever a result is assigned, and report it
86-
didSet { result.map(report) }
87-
}
88-
89-
// Report success or error based on result
90-
private func report(result: Result<Value, Failure>) {
91-
switch result {
97+
98+
/// Stores the result and fires every currently-registered callback.
99+
///
100+
/// Repeated calls are allowed: in-tree callers reuse a single `Fulfill` as a
101+
/// broadcast event signal. Each call overwrites `result` and fires the matching
102+
/// branch against a snapshot of the callback list taken under the lock.
103+
/// Callbacks fire outside the critical section so re-entrant registrations on
104+
/// the same instance cannot deadlock.
105+
fileprivate func setResult(_ newResult: Result<Value, Failure>) {
106+
let snapshot: (successes: [(Value) -> Void], errors: [(Failure) -> Void]) = stateQueue.sync {
107+
result = newResult
108+
return (successCallbacks, errorCallbacks)
109+
}
110+
111+
switch newResult {
92112
case let .success(value):
93-
successCallbacks.forEach { $0(value) }
113+
snapshot.successes.forEach { $0(value) }
94114
case let .failure(error):
95-
errorCallbacks.forEach { $0(error) }
115+
snapshot.errors.forEach { $0(error) }
96116
}
97117
}
98118
}
@@ -101,7 +121,7 @@ public class Pending<Value, Failure> where Failure: Error {
101121
public class FailPending<Value, Failure: Error>: Pending<Value, Failure> {
102122
public init(error: Failure) {
103123
super.init()
104-
self.result = .failure(error)
124+
setResult(.failure(error))
105125
}
106126
}
107127

@@ -199,27 +219,25 @@ public class Fulfill<Value, Failure>: Pending<Value, Failure> where Failure: Err
199219
ITBDebug()
200220
super.init()
201221
if let value = value {
202-
result = Result.success(value)
203-
} else {
204-
result = nil
222+
setResult(.success(value))
205223
}
206224
}
207-
225+
208226
public init(error: Failure) {
209227
ITBDebug()
210228
super.init()
211-
result = Result.failure(error)
229+
setResult(.failure(error))
212230
}
213231

214232
deinit {
215233
ITBDebug()
216234
}
217-
235+
218236
public func resolve(with value: Value) {
219-
result = .success(value)
237+
setResult(.success(value))
220238
}
221-
239+
222240
public func reject(with error: Failure) {
223-
result = .failure(error)
241+
setResult(.failure(error))
224242
}
225243
}

0 commit comments

Comments
 (0)