Skip to content

Update .cancellable implementation to fix #946 #949

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 7, 2022
Merged

Update .cancellable implementation to fix #946 #949

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d9d9bbd
Subscribe to upstream publisher instead of PassthroughSubject
iampatbrown Jan 6, 2022
479f5c1
Fixed operator order
iampatbrown Jan 6, 2022
e1ead6f
Replaced RunLoop.immediate with RunLoop.test
iampatbrown Jan 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions Examples/VoiceMemos/VoiceMemosTests/VoiceMemosTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ class VoiceMemosTests: XCTestCase {
environment.audioRecorder.startRecording = { _ in
audioRecorderSubject.eraseToEffect()
}
environment.mainRunLoop = .immediate
environment.mainRunLoop = self.mainRunLoop.eraseToAnyScheduler()
environment.temporaryDirectory = { .init(fileURLWithPath: "/tmp") }
environment.uuid = { UUID(uuidString: "DEADBEEF-DEAD-BEEF-DEAD-BEEFDEADBEEF")! }

Expand All @@ -124,6 +124,7 @@ class VoiceMemosTests: XCTestCase {
)

store.send(.recordButtonTapped)
Copy link
Contributor Author

@iampatbrown iampatbrown Jan 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.recordButtonTapped ends up returning a merged effect

swift-composable-architecture/Examples/VoiceMemos/VoiceMemos/VoiceMemos.swift

Lines 70 to 76 in 313dd21

return .merge(
environment.audioRecorder.startRecording(url)
.catchToEffect(VoiceMemosAction.audioRecorder),
Effect.timer(id: TimerId(), every: 1, tolerance: .zero, on: environment.mainRunLoop)
.map { _ in .currentRecordingTimerUpdated }
)

using RunLoop.immediate results in a failed test because .currentRecordingTimerUpdated now also gets received once before .audioRecorder(.failure(.couldntActivateAudioSession)) is received. I'm unsure if this is an undesired result of the change.

Adding the following test to TimerTests.swift demonstrates the difference in behaviour.

 func testTimerImmediate() {
    let scheduler = DispatchQueue.immediate

    var count = 0

    Effect.timer(id: 1, every: .seconds(1), on: scheduler)
      .sink { _ in count += 1 }
      .store(in: &self.cancellables)

    XCTAssertNoDifference(count, 1) // This PR

    // XCTAssertNoDifference(count, 0) // Current implementation
  }

Copy link
Contributor Author

@iampatbrown iampatbrown Jan 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the current implementation, the timer is triggered as soon as the PassThroughSubject subscribes to it, essentially dropping the value before the effect is subscribed to downstream.

swift-composable-architecture/Sources/ComposableArchitecture/Effects/Cancellation.swift

Lines 36 to 37 in 313dd21

let subject = PassthroughSubject<Output, Failure>()
let cancellable = self.subscribe(subject)

So the timer auto connects and outputs a value at line 37 which is lost before returning the effect.

In this PR the timer auto connects at .sink { _ in count += 1 }

Copy link
Member

@stephencelis stephencelis Jan 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting find! I think overall this PR is an improvement in the expectation that at least 1 thing emits from a timer running on an immediate scheduler, even if it is a breaking change.

self.mainRunLoop.advance(by: 0.5)
store.receive(.recordPermissionResponse(true)) {
$0.audioRecorderPermission = .allowed
$0.currentRecording = .init(
Expand All @@ -133,6 +134,7 @@ class VoiceMemosTests: XCTestCase {
)
}
audioRecorderSubject.send(completion: .failure(.couldntActivateAudioSession))
self.mainRunLoop.advance(by: 0.5)
store.receive(.audioRecorder(.failure(.couldntActivateAudioSession))) {
$0.alert = .init(title: .init("Voice memo recording failed."))
$0.currentRecording = nil
Expand Down Expand Up @@ -189,7 +191,7 @@ class VoiceMemosTests: XCTestCase {
func testPlayMemoFailure() {
var environment = VoiceMemosEnvironment.failing
environment.audioPlayer.play = { _ in Effect(error: .decodeErrorDidOccur) }
environment.mainRunLoop = .immediate
environment.mainRunLoop = self.mainRunLoop.eraseToAnyScheduler()

let url = URL(string: "https://www.pointfree.co/functions")!
let store = TestStore(
Expand Down Expand Up @@ -278,7 +280,7 @@ class VoiceMemosTests: XCTestCase {
var environment = VoiceMemosEnvironment.failing
environment.audioPlayer.play = { _ in .none }
environment.audioPlayer.stop = { .none }
environment.mainRunLoop = .immediate
environment.mainRunLoop = self.mainRunLoop.eraseToAnyScheduler()

let store = TestStore(
initialState: VoiceMemosState(
Expand Down
17 changes: 8 additions & 9 deletions Sources/ComposableArchitecture/Effects/Cancellation.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,16 @@ extension Effect {
/// canceled before starting this new one.
/// - Returns: A new effect that is capable of being canceled by an identifier.
public func cancellable(id: AnyHashable, cancelInFlight: Bool = false) -> Effect {
let effect = Deferred { () -> Publishers.HandleEvents<PassthroughSubject<Output, Failure>> in
let effect = Deferred { () -> Publishers.HandleEvents<Publishers.PrefixUntilOutput<Self, PassthroughSubject<Void, Never>>> in
cancellablesLock.lock()
defer { cancellablesLock.unlock() }

let subject = PassthroughSubject<Output, Failure>()
let cancellable = self.subscribe(subject)
let cancellationSubject = PassthroughSubject<Void, Never>()

var cancellationCancellable: AnyCancellable!
cancellationCancellable = AnyCancellable {
cancellablesLock.sync {
subject.send(completion: .finished)
cancellable.cancel()
cancellationSubject.send(())
cancellationCancellables[id]?.remove(cancellationCancellable)
if cancellationCancellables[id]?.isEmpty == .some(true) {
cancellationCancellables[id] = nil
Expand All @@ -52,10 +50,11 @@ extension Effect {
cancellationCancellable
)

return subject.handleEvents(
receiveCompletion: { _ in cancellationCancellable.cancel() },
receiveCancel: cancellationCancellable.cancel
)
return self.prefix(untilOutputFrom: cancellationSubject)
.handleEvents(
receiveCompletion: { _ in cancellationCancellable.cancel() },
receiveCancel: cancellationCancellable.cancel
)
}
.eraseToEffect()

Expand Down
16 changes: 16 additions & 0 deletions Tests/ComposableArchitectureTests/EffectCancellationTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,20 @@ final class EffectCancellationTests: XCTestCase {
scheduler.advance(by: 1)
XCTAssertNoDifference(expectedOutput, [])
}

func testNestedMergeCancellation() {
let effect = Effect<Int, Never>.merge(
(1...2).publisher
.eraseToEffect()
.cancellable(id: 1)
)
.cancellable(id: 2)

var output: [Int] = []
effect
.sink { output.append($0) }
.store(in: &cancellables)

XCTAssertEqual(output, [1, 2])
}
}