From d0758999f79c30afd8a38fb92875ec38811fc7c6 Mon Sep 17 00:00:00 2001 From: Stephen Celis Date: Tue, 26 May 2020 18:42:57 -0400 Subject: [PATCH 1/3] Avoid simultaneous access crash on double-cancel --- .../Effects/Cancellation.swift | 32 ++++++------------- .../Internal/Locking.swift | 4 +-- .../EffectCancellationTests.swift | 27 ++++++++++++++-- 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/Sources/ComposableArchitecture/Effects/Cancellation.swift b/Sources/ComposableArchitecture/Effects/Cancellation.swift index 7cf8f03e27d8..7ec4ca32ae78 100644 --- a/Sources/ComposableArchitecture/Effects/Cancellation.swift +++ b/Sources/ComposableArchitecture/Effects/Cancellation.swift @@ -29,40 +29,25 @@ extension Effect { public func cancellable(id: AnyHashable, cancelInFlight: Bool = false) -> Effect { return Deferred { () -> Publishers.HandleEvents> in let subject = PassthroughSubject() - let uuid = UUID() - - var isCleaningUp = false cancellablesLock.sync { if cancelInFlight { - cancellationCancellables[id]?.forEach { _, cancellable in cancellable.cancel() } + cancellationCancellables[id]?.forEach { cancellable in cancellable.cancel() } cancellationCancellables[id] = nil } let cancellable = self.subscribe(subject) - cancellationCancellables[id] = cancellationCancellables[id] ?? [:] - cancellationCancellables[id]?[uuid] = AnyCancellable { + AnyCancellable { cancellable.cancel() - if !isCleaningUp { - subject.send(completion: .finished) - } - } - } - - func cleanup() { - isCleaningUp = true - cancellablesLock.sync { - cancellationCancellables[id]?[uuid] = nil - if cancellationCancellables[id]?.isEmpty == true { - cancellationCancellables[id] = nil - } + subject.send(completion: .finished) } + .store(in: &cancellationCancellables[id, default: []]) } return subject.handleEvents( - receiveCompletion: { _ in cleanup() }, - receiveCancel: cleanup + receiveCompletion: { _ in cancellablesLock.sync { cancellationCancellables[id] = nil } }, + receiveCancel: { cancellablesLock.sync { cancellationCancellables[id] = nil } } ) } .eraseToEffect() @@ -76,12 +61,13 @@ extension Effect { public static func cancel(id: AnyHashable) -> Effect { .fireAndForget { cancellablesLock.sync { - cancellationCancellables[id]?.forEach { _, cancellable in cancellable.cancel() } + cancellationCancellables[id]?.forEach { cancellable in cancellable.cancel() } cancellationCancellables[id] = nil } } } } -var cancellationCancellables: [AnyHashable: [UUID: AnyCancellable]] = [:] +var cancellationCancellables: [AnyHashable: Set] = [:] let cancellablesLock = NSRecursiveLock() +var isCancelling: Set = [] diff --git a/Sources/ComposableArchitecture/Internal/Locking.swift b/Sources/ComposableArchitecture/Internal/Locking.swift index b96ecf7aa66a..6feff1d1929f 100644 --- a/Sources/ComposableArchitecture/Internal/Locking.swift +++ b/Sources/ComposableArchitecture/Internal/Locking.swift @@ -11,9 +11,9 @@ extension UnsafeMutablePointer where Pointee == os_unfair_lock_s { extension NSRecursiveLock { @inlinable - func sync(work: () -> Void) { + func sync(work: () -> R) -> R { self.lock() defer { self.unlock() } - work() + return work() } } diff --git a/Tests/ComposableArchitectureTests/EffectCancellationTests.swift b/Tests/ComposableArchitectureTests/EffectCancellationTests.swift index 6acddd50b6b3..d06a2231693b 100644 --- a/Tests/ComposableArchitectureTests/EffectCancellationTests.swift +++ b/Tests/ComposableArchitectureTests/EffectCancellationTests.swift @@ -223,17 +223,40 @@ final class EffectCancellationTests: XCTestCase { let expectation = self.expectation(description: "wait") effect - .sink(receiveCompletion: { _ in expectation.fulfill() }, receiveValue: { _ in }) + .sink(receiveCompletion: { _ in + print("completed") + expectation.fulfill() + + }, receiveValue: { _ in }) .store(in: &self.cancellables) self.wait(for: [expectation], timeout: 999) XCTAssertTrue(cancellationCancellables.isEmpty) } + + func testNestedCancels() { + var effect = Empty(completeImmediately: false) + .eraseToEffect() + .cancellable(id: 1) + + for _ in 1 ... .random(in: 1...1_000) { + effect = effect.cancellable(id: 1) + } + + effect + .sink(receiveValue: { _ in }) + .store(in: &cancellables) + + cancellables.removeAll() + + XCTAssertEqual([:], cancellationCancellables) + XCTAssertEqual([], isCancelling) + } } func resetCancellables() { for (id, _) in cancellationCancellables { - cancellationCancellables[id] = [:] + cancellationCancellables[id] = [] } cancellationCancellables = [:] } From c68308a85f53ea49a800ead3a30c3e5b3556746d Mon Sep 17 00:00:00 2001 From: Stephen Celis Date: Tue, 26 May 2020 19:02:36 -0400 Subject: [PATCH 2/3] fixes --- .../Effects/Cancellation.swift | 13 +++++++++++-- .../EffectCancellationTests.swift | 15 ++++++--------- 2 files changed, 17 insertions(+), 11 deletions(-) diff --git a/Sources/ComposableArchitecture/Effects/Cancellation.swift b/Sources/ComposableArchitecture/Effects/Cancellation.swift index 7ec4ca32ae78..d74739a6c560 100644 --- a/Sources/ComposableArchitecture/Effects/Cancellation.swift +++ b/Sources/ComposableArchitecture/Effects/Cancellation.swift @@ -45,9 +45,18 @@ extension Effect { .store(in: &cancellationCancellables[id, default: []]) } + func cleanUp() { + cancellablesLock.sync { + guard !isCancelling.contains(id) else { return } + isCancelling.insert(id) + defer { isCancelling.remove(id) } + cancellationCancellables[id] = nil + } + } + return subject.handleEvents( - receiveCompletion: { _ in cancellablesLock.sync { cancellationCancellables[id] = nil } }, - receiveCancel: { cancellablesLock.sync { cancellationCancellables[id] = nil } } + receiveCompletion: { _ in cleanUp() }, + receiveCancel: cleanUp ) } .eraseToEffect() diff --git a/Tests/ComposableArchitectureTests/EffectCancellationTests.swift b/Tests/ComposableArchitectureTests/EffectCancellationTests.swift index d06a2231693b..1776a1c5fb89 100644 --- a/Tests/ComposableArchitectureTests/EffectCancellationTests.swift +++ b/Tests/ComposableArchitectureTests/EffectCancellationTests.swift @@ -223,11 +223,7 @@ final class EffectCancellationTests: XCTestCase { let expectation = self.expectation(description: "wait") effect - .sink(receiveCompletion: { _ in - print("completed") - expectation.fulfill() - - }, receiveValue: { _ in }) + .sink(receiveCompletion: { _ in expectation.fulfill() }, receiveValue: { _ in }) .store(in: &self.cancellables) self.wait(for: [expectation], timeout: 999) @@ -250,13 +246,14 @@ final class EffectCancellationTests: XCTestCase { cancellables.removeAll() XCTAssertEqual([:], cancellationCancellables) - XCTAssertEqual([], isCancelling) } } func resetCancellables() { - for (id, _) in cancellationCancellables { - cancellationCancellables[id] = [] + cancellablesLock.sync { + for (id, _) in cancellationCancellables { + cancellationCancellables[id] = [] + } + cancellationCancellables = [:] } - cancellationCancellables = [:] } From f9cd5da79178f0b25a2e3176ddb76618d10816dc Mon Sep 17 00:00:00 2001 From: Stephen Celis Date: Tue, 26 May 2020 19:03:35 -0400 Subject: [PATCH 3/3] fix --- Tests/ComposableArchitectureTests/EffectCancellationTests.swift | 1 + 1 file changed, 1 insertion(+) diff --git a/Tests/ComposableArchitectureTests/EffectCancellationTests.swift b/Tests/ComposableArchitectureTests/EffectCancellationTests.swift index 1776a1c5fb89..098e3f3c9c63 100644 --- a/Tests/ComposableArchitectureTests/EffectCancellationTests.swift +++ b/Tests/ComposableArchitectureTests/EffectCancellationTests.swift @@ -246,6 +246,7 @@ final class EffectCancellationTests: XCTestCase { cancellables.removeAll() XCTAssertEqual([:], cancellationCancellables) + XCTAssertEqual([], isCancelling) } }