Skip to content

Commit 414e4fa

Browse files
stephencelismbrandonw
stephencelis
authored and
mbrandonw
committed
Address effect cancellation sendability (#3326)
* Address effect cancellation sendability * fix * wip * wip
1 parent 53d5389 commit 414e4fa

File tree

4 files changed

+85
-75
lines changed

4 files changed

+85
-75
lines changed

‎Sources/ComposableArchitecture/Effects/Cancellation.swift

+69-67
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import Combine
1+
@preconcurrency import Combine
22
import Foundation
33

44
extension Effect {
@@ -49,34 +49,35 @@ extension Effect {
4949
AnyPublisher<Action, Never>, PassthroughSubject<Void, Never>
5050
>
5151
> in
52-
_cancellablesLock.lock()
53-
defer { _cancellablesLock.unlock() }
54-
55-
if cancelInFlight {
56-
_cancellationCancellables.cancel(id: id, path: navigationIDPath)
57-
}
58-
59-
let cancellationSubject = PassthroughSubject<Void, Never>()
60-
61-
var cancellable: AnyCancellable!
62-
cancellable = AnyCancellable {
63-
_cancellablesLock.sync {
64-
cancellationSubject.send(())
65-
cancellationSubject.send(completion: .finished)
66-
_cancellationCancellables.remove(cancellable, at: id, path: navigationIDPath)
52+
_cancellationCancellables.withValue {
53+
if cancelInFlight {
54+
$0.cancel(id: id, path: navigationIDPath)
6755
}
68-
}
6956

70-
return publisher.prefix(untilOutputFrom: cancellationSubject)
71-
.handleEvents(
72-
receiveSubscription: { _ in
73-
_cancellablesLock.sync {
74-
_cancellationCancellables.insert(cancellable, at: id, path: navigationIDPath)
57+
let cancellationSubject = PassthroughSubject<Void, Never>()
58+
59+
let cancellable = LockIsolated<AnyCancellable?>(nil)
60+
cancellable.setValue(
61+
AnyCancellable {
62+
_cancellationCancellables.withValue {
63+
cancellationSubject.send(())
64+
cancellationSubject.send(completion: .finished)
65+
$0.remove(cancellable.value!, at: id, path: navigationIDPath)
7566
}
76-
},
77-
receiveCompletion: { _ in cancellable.cancel() },
78-
receiveCancel: cancellable.cancel
67+
}
7968
)
69+
70+
return publisher.prefix(untilOutputFrom: cancellationSubject)
71+
.handleEvents(
72+
receiveSubscription: { _ in
73+
_cancellationCancellables.withValue {
74+
$0.insert(cancellable.value!, at: id, path: navigationIDPath)
75+
}
76+
},
77+
receiveCompletion: { _ in cancellable.value!.cancel() },
78+
receiveCancel: cancellable.value!.cancel
79+
)
80+
}
8081
}
8182
.eraseToAnyPublisher()
8283
)
@@ -101,7 +102,7 @@ extension Effect {
101102
/// - Parameter id: An effect identifier.
102103
/// - Returns: A new effect that will cancel any currently in-flight effect with the given
103104
/// identifier.
104-
public static func cancel<ID: Hashable>(id: ID) -> Self {
105+
public static func cancel(id: some Hashable & Sendable) -> Self {
105106
let dependencies = DependencyValues._current
106107
@Dependency(\.navigationIDPath) var navigationIDPath
107108
// NB: Ideally we'd return a `Deferred` wrapping an `Empty(completeImmediately: true)`, but
@@ -110,8 +111,8 @@ extension Effect {
110111
// trickery to make sure the deferred publisher completes.
111112
return .publisher { () -> Publishers.CompactMap<Just<Action?>, Action> in
112113
DependencyValues.$_current.withValue(dependencies) {
113-
_cancellablesLock.sync {
114-
_cancellationCancellables.cancel(id: id, path: navigationIDPath)
114+
_cancellationCancellables.withValue {
115+
$0.cancel(id: id, path: navigationIDPath)
115116
}
116117
}
117118
return Just<Action?>(nil).compactMap { $0 }
@@ -163,26 +164,27 @@ extension Effect {
163164
/// - operation: An async operation.
164165
/// - Throws: An error thrown by the operation.
165166
/// - Returns: A value produced by operation.
166-
public func withTaskCancellation<ID: Hashable, T>(
167-
id: ID,
167+
public func withTaskCancellation<T: Sendable>(
168+
id: some Hashable & Sendable,
168169
cancelInFlight: Bool = false,
169170
isolation: isolated (any Actor)? = #isolation,
170-
operation: sending @escaping @isolated(any) () async throws -> sending T
171+
operation: @escaping @Sendable () async throws -> T
171172
) async rethrows -> T {
172173
@Dependency(\.navigationIDPath) var navigationIDPath
173174

174-
let (cancellable, task) = _cancellablesLock.sync { () -> (AnyCancellable, Task<T, Error>) in
175-
if cancelInFlight {
176-
_cancellationCancellables.cancel(id: id, path: navigationIDPath)
175+
let (cancellable, task): (AnyCancellable, Task<T, Error>) = _cancellationCancellables
176+
.withValue {
177+
if cancelInFlight {
178+
$0.cancel(id: id, path: navigationIDPath)
179+
}
180+
let task = Task { try await operation() }
181+
let cancellable = AnyCancellable { task.cancel() }
182+
$0.insert(cancellable, at: id, path: navigationIDPath)
183+
return (cancellable, task)
177184
}
178-
let task = Task { try await operation() }
179-
let cancellable = AnyCancellable { task.cancel() }
180-
_cancellationCancellables.insert(cancellable, at: id, path: navigationIDPath)
181-
return (cancellable, task)
182-
}
183185
defer {
184-
_cancellablesLock.sync {
185-
_cancellationCancellables.remove(cancellable, at: id, path: navigationIDPath)
186+
_cancellationCancellables.withValue {
187+
$0.remove(cancellable, at: id, path: navigationIDPath)
186188
}
187189
}
188190
do {
@@ -193,25 +195,26 @@ extension Effect {
193195
}
194196
#else
195197
@_unsafeInheritExecutor
196-
public func withTaskCancellation<ID: Hashable, T: Sendable>(
197-
id: ID,
198+
public func withTaskCancellation<T: Sendable>(
199+
id: some Hashable,
198200
cancelInFlight: Bool = false,
199201
operation: @Sendable @escaping () async throws -> T
200202
) async rethrows -> T {
201203
@Dependency(\.navigationIDPath) var navigationIDPath
202204

203-
let (cancellable, task) = _cancellablesLock.sync { () -> (AnyCancellable, Task<T, Error>) in
204-
if cancelInFlight {
205-
_cancellationCancellables.cancel(id: id, path: navigationIDPath)
205+
let (cancellable, task): (AnyCancellable, Task<T, Error>) = _cancellationCancellables
206+
.withValue {
207+
if cancelInFlight {
208+
$0.cancel(id: id, path: navigationIDPath)
209+
}
210+
let task = Task { try await operation() }
211+
let cancellable = AnyCancellable { task.cancel() }
212+
$0.insert(cancellable, at: id, path: navigationIDPath)
213+
return (cancellable, task)
206214
}
207-
let task = Task { try await operation() }
208-
let cancellable = AnyCancellable { task.cancel() }
209-
_cancellationCancellables.insert(cancellable, at: id, path: navigationIDPath)
210-
return (cancellable, task)
211-
}
212215
defer {
213-
_cancellablesLock.sync {
214-
_cancellationCancellables.remove(cancellable, at: id, path: navigationIDPath)
216+
_cancellationCancellables.withValue {
217+
$0.remove(cancellable, at: id, path: navigationIDPath)
215218
}
216219
}
217220
do {
@@ -226,11 +229,11 @@ extension Task<Never, Never> {
226229
/// Cancel any currently in-flight operation with the given identifier.
227230
///
228231
/// - Parameter id: An identifier.
229-
public static func cancel<ID: Hashable>(id: ID) {
232+
public static func cancel(id: some Hashable & Sendable) {
230233
@Dependency(\.navigationIDPath) var navigationIDPath
231234

232-
return _cancellablesLock.sync {
233-
_cancellationCancellables.cancel(id: id, path: navigationIDPath)
235+
return _cancellationCancellables.withValue {
236+
$0.cancel(id: id, path: navigationIDPath)
234237
}
235238
}
236239
}
@@ -240,15 +243,14 @@ extension Task<Never, Never> {
240243
let id: AnyHashable
241244
let navigationIDPath: NavigationIDPath
242245

243-
init<ID: Hashable>(id: ID, navigationIDPath: NavigationIDPath) {
246+
init(id: some Hashable, navigationIDPath: NavigationIDPath) {
244247
self.discriminator = ObjectIdentifier(type(of: id))
245248
self.id = id
246249
self.navigationIDPath = navigationIDPath
247250
}
248251
}
249252

250-
@_spi(Internals) public var _cancellationCancellables = CancellablesCollection()
251-
private let _cancellablesLock = NSRecursiveLock()
253+
@_spi(Internals) public let _cancellationCancellables = LockIsolated(CancellablesCollection())
252254

253255
@rethrows
254256
private protocol _ErrorMechanism {
@@ -273,9 +275,9 @@ extension Result: _ErrorMechanism {}
273275
public class CancellablesCollection {
274276
var storage: [_CancelID: Set<AnyCancellable>] = [:]
275277

276-
func insert<ID: Hashable>(
278+
func insert(
277279
_ cancellable: AnyCancellable,
278-
at id: ID,
280+
at id: some Hashable,
279281
path: NavigationIDPath
280282
) {
281283
for navigationIDPath in path.prefixes {
@@ -284,9 +286,9 @@ public class CancellablesCollection {
284286
}
285287
}
286288

287-
func remove<ID: Hashable>(
289+
func remove(
288290
_ cancellable: AnyCancellable,
289-
at id: ID,
291+
at id: some Hashable,
290292
path: NavigationIDPath
291293
) {
292294
for navigationIDPath in path.prefixes {
@@ -298,17 +300,17 @@ public class CancellablesCollection {
298300
}
299301
}
300302

301-
func cancel<ID: Hashable>(
302-
id: ID,
303+
func cancel(
304+
id: some Hashable,
303305
path: NavigationIDPath
304306
) {
305307
let cancelID = _CancelID(id: id, navigationIDPath: path)
306308
self.storage[cancelID]?.forEach { $0.cancel() }
307309
self.storage[cancelID] = nil
308310
}
309311

310-
func exists<ID: Hashable>(
311-
at id: ID,
312+
func exists(
313+
at id: some Hashable,
312314
path: NavigationIDPath
313315
) -> Bool {
314316
self.storage[_CancelID(id: id, navigationIDPath: path)] != nil

‎Sources/swift-composable-architecture-benchmark/StoreSuite.swift

+2-2
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ let storeSuite = BenchmarkSuite(name: "Store") { suite in
1616
}
1717
} tearDown: {
1818
precondition(count(of: store.withState { $0 }, level: level) == 1)
19-
_cancellationCancellables.removeAll()
19+
_cancellationCancellables.withValue { $0.removeAll() }
2020
}
2121
}
2222
for level in 1...levels {
@@ -28,7 +28,7 @@ let storeSuite = BenchmarkSuite(name: "Store") { suite in
2828
}
2929
} tearDown: {
3030
precondition(count(of: store.withState { $0 }, level: level) == 0)
31-
_cancellationCancellables.removeAll()
31+
_cancellationCancellables.withValue { $0.removeAll() }
3232
}
3333
}
3434
}

‎Tests/ComposableArchitectureTests/EffectCancellationTests.swift

+10-4
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,10 @@ final class EffectCancellationTests: BaseTCATestCase {
292292

293293
for await _ in Effect.send(1).cancellable(id: id).actions {}
294294

295-
XCTAssertEqual(_cancellationCancellables.exists(at: id, path: NavigationIDPath()), false)
295+
XCTAssertEqual(
296+
_cancellationCancellables.withValue { $0.exists(at: id, path: NavigationIDPath()) },
297+
false
298+
)
296299
}
297300

298301
func testCancellablesCleanUp_OnCancel() async {
@@ -315,7 +318,10 @@ final class EffectCancellationTests: BaseTCATestCase {
315318

316319
await task.value
317320

318-
XCTAssertEqual(_cancellationCancellables.exists(at: id, path: NavigationIDPath()), false)
321+
XCTAssertEqual(
322+
_cancellationCancellables.withValue { $0.exists(at: id, path: NavigationIDPath()) },
323+
false
324+
)
319325
}
320326

321327
func testConcurrentCancels() {
@@ -363,7 +369,7 @@ final class EffectCancellationTests: BaseTCATestCase {
363369

364370
for id in ids {
365371
XCTAssertEqual(
366-
_cancellationCancellables.exists(at: id, path: NavigationIDPath()),
372+
_cancellationCancellables.withValue { $0.exists(at: id, path: NavigationIDPath()) },
367373
false,
368374
"cancellationCancellables should not contain id \(id)"
369375
)
@@ -396,7 +402,7 @@ final class EffectCancellationTests: BaseTCATestCase {
396402

397403
for id in ids {
398404
XCTAssertEqual(
399-
_cancellationCancellables.exists(at: id, path: NavigationIDPath()),
405+
_cancellationCancellables.withValue { $0.exists(at: id, path: NavigationIDPath()) },
400406
false,
401407
"cancellationCancellables should not contain id \(id)"
402408
)

‎Tests/ComposableArchitectureTests/Internal/BaseTCATestCase.swift

+4-2
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,10 @@ import XCTest
44
class BaseTCATestCase: XCTestCase {
55
override func tearDown() async throws {
66
try await super.tearDown()
7-
XCTAssertEqual(_cancellationCancellables.count, 0, "\(self)")
8-
_cancellationCancellables.removeAll()
7+
_cancellationCancellables.withValue { [description = "\(self)"] in
8+
XCTAssertEqual($0.count, 0, description)
9+
$0.removeAll()
10+
}
911
await MainActor.run {
1012
Logger.shared.isEnabled = false
1113
Logger.shared.clear()

0 commit comments

Comments
 (0)