Merge pull request #267 from ReactiveCocoa/signal-deadlock-fix

Fixed a race condition in `Signal` terminal event handling.

Author: andersio

.travis.yml

@@ -9,6 +9,7 @@ branches:
     - master
     # Credit: @Omnikron13, https://github.com/mojombo/semver/issues/32
     - /^(\d+\.\d+\.\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?$/
+    - /^hotfix-(\d+\.\d+\.\d+)(?:-([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?(?:\+([0-9A-Za-z-]+(?:\.[0-9A-Za-z-]+)*))?$/ 
 before_script:
   - git submodule update --init --recursive
 script:

Sources/Signal.swift

@@ -313,13 +313,24 @@ public final class Signal<Value, Error: Swift.Error> {
 			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 {

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<Bool>? = MutableProperty(false)
 				weak var weakProperty = property
@@ -114,6 +114,29 @@ 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()) }
+
+				// Fixed in #267. (https://github.com/ReactiveCocoa/ReactiveSwift/pull/267)
+				//
+				// The deadlock happened as the observer disposable releases the closure
+				// `{ _ in viewModel }` here without releasing the mapped signal's
+				// `updateLock` first. The deinitialization of the closure triggered the
+				// propagation of terminal event of the `Action`, which eventually hit
+				// the mapped signal and attempted to acquire `updateLock` to transition
+				// the signal's state.
+				action1.values
+					.flatMap(.latest) { viewModel in viewModel.action2.values.map { _ in viewModel } }
+					.observeValues { _ in }
+
+				action1.apply().start()
+				action1.apply().start()
+			}
+
 			if #available(macOS 10.10, *) {
 				it("should not loop indefinitely") {
 					let condition = MutableProperty(1)