diff --git a/Sources/Signal.swift b/Sources/Signal.swift index 073d469e4..50ba2defe 100644 --- a/Sources/Signal.swift +++ b/Sources/Signal.swift @@ -313,13 +313,24 @@ public final class Signal { return ActionDisposable { [weak self] in if let s = self { s.updateLock.lock() + if case let .alive(snapshot) = s.state { var observers = snapshot.observers observers.remove(using: token) - s.state = .alive(AliveState(observers: observers, - retaining: observers.isEmpty ? nil : self)) + + // Ensure the old signal state snapshot does not deinitialize before + // `updateLock` is released. Otherwise, it might result in a + // deadlock in cases where a `Signal` legitimately receives terminal + // events recursively as a result of the deinitialization of the + // snapshot. + withExtendedLifetime(snapshot) { + s.state = .alive(AliveState(observers: observers, + retaining: observers.isEmpty ? nil : self)) + s.updateLock.unlock() + } + } else { + s.updateLock.unlock() } - s.updateLock.unlock() } } } else { diff --git a/Tests/ReactiveSwiftTests/ActionSpec.swift b/Tests/ReactiveSwiftTests/ActionSpec.swift index d739fa26b..b7fd27411 100755 --- a/Tests/ReactiveSwiftTests/ActionSpec.swift +++ b/Tests/ReactiveSwiftTests/ActionSpec.swift @@ -58,7 +58,7 @@ class ActionSpec: QuickSpec { action.errors.observeValues { errors.append($0) } action.completed.observeValues { completedCount += 1 } } - + it("should retain the state property") { var property: MutableProperty? = MutableProperty(false) weak var weakProperty = property @@ -114,6 +114,21 @@ class ActionSpec: QuickSpec { expect(action.isExecuting.value) == false } + it("should not deadlock") { + final class ViewModel { + let action2 = Action<(), (), NoError> { SignalProducer(value: ()) } + } + + let action1 = Action<(), ViewModel, NoError> { SignalProducer(value: ViewModel()) } + + action1.values + .flatMap(.latest) { viewModel in viewModel.action2.values.map { _ in viewModel } } + .observeValues { _ in } + + action1.apply().start() + action1.apply().start() + } + describe("completed") { beforeEach { enabled.value = true