Expand feedback support to Action.isEnabled.

Author: andersio

CHANGELOG.md

@@ -1,6 +1,10 @@
 # master
 *Please add new entries at the top.*
 
+1. Feedbacks from `isEnabled` to the state of the same `Action` no longer deadlocks if it does not constitute an infinite feedback loop. (#481, kudos to @andersio)
+
+   Note that `isExecuting` already supports `Action` state feedback, and legitimate feedback loops would still deadlock.
+
 # 2.0.0-rc.2
 1. Fixed a deadlock upon disposal when combining operators, i.e. `zip` and `combineLatest`, are used. (#471, kudos to @stevebrambilla for catching the bug)
 

Sources/Action.swift

@@ -98,15 +98,6 @@ public final class Action<Input, Output, Error: Swift.Error> {
 		deinitToken = Lifetime.Token()
 		lifetime = Lifetime(deinitToken)
 
-		let actionState = MutableProperty(ActionState<State.Value>(isUserEnabled: true, isExecuting: false, value: state.value))
-		self.isEnabled = actionState.map { $0.isEnabled }
-
-		// `isExecuting` has its own backing so that when the observer of `isExecuting`
-		// synchronously affects the action state, the signal of the action state does not
-		// deadlock due to the recursion.
-		let isExecuting = MutableProperty(false)
-		self.isExecuting = Property(capturing: isExecuting)
-
 		// `Action` retains its state property.
 		lifetime.observeEnded { _ = state }
 
@@ -117,8 +108,34 @@ public final class Action<Input, Output, Error: Swift.Error> {
 		errors = events.filterMap { $0.error }
 		completed = events.filterMap { $0.isCompleted ? () : nil }
 
+		let actionState = MutableProperty(ActionState<State.Value>(isUserEnabled: true, isExecuting: false, value: state.value))
+
+		// `isEnabled` and `isExecuting` have their own backing so that when the observers
+		// of these synchronously affects the action state, the signal of the action state
+		// does not deadlock due to the recursion.
+		let isExecuting = MutableProperty(false)
+		self.isExecuting = Property(capturing: isExecuting)
+		let isEnabled = MutableProperty(actionState.value.isEnabled)
+		self.isEnabled = Property(capturing: isEnabled)
+
+		func modifyActionState<Result>(_ action: (inout ActionState<State.Value>) throws -> Result) rethrows -> Result {
+			return try actionState.begin { storage in
+				let oldState = storage.value
+				defer {
+					let newState = storage.value
+					if oldState.isEnabled != newState.isEnabled {
+						isEnabled.value = newState.isEnabled
+					}
+					if oldState.isExecuting != newState.isExecuting {
+						isExecuting.value = newState.isExecuting
+					}
+				}
+				return try storage.modify(action)
+			}
+		}
+
 		let disposable = state.producer.startWithValues { value in
-			actionState.modify { state in
+			modifyActionState { state in
 				state.value = value
 				state.isUserEnabled = isUserEnabled(value)
 			}
@@ -128,21 +145,12 @@ public final class Action<Input, Output, Error: Swift.Error> {
 
 		self.execute = { action, input in
 			return SignalProducer { observer, lifetime in
-				var notifiesExecutionState = false
-
-				func didSet() {
-					if notifiesExecutionState {
-						isExecuting.value = true
-					}
-				}
-
-				let latestState: State.Value? = actionState.modify(didSet: didSet) { state in
+				let latestState: State.Value? = modifyActionState { state in
 					guard state.isEnabled else {
 						return nil
 					}
 
 					state.isExecuting = true
-					notifiesExecutionState = true
 					return state.value
 				}
 
@@ -159,10 +167,7 @@ public final class Action<Input, Output, Error: Swift.Error> {
 
 				lifetime.observeEnded {
 					interruptHandle.dispose()
-
-					actionState.modify(didSet: { isExecuting.value = false }) { state in
-						state.isExecuting = false
-					}
+					modifyActionState { $0.isExecuting = false }
 				}
 			}
 		}

Sources/Property.swift

@@ -554,10 +554,13 @@ public final class Property<Value>: PropertyProtocol {
 					return observer.action(event)
 				}
 
-				box.modify(didSet: { _ in observer.action(event) }) { value in
-					if let newValue = event.value {
-						value = newValue
+				box.begin { storage in
+					storage.modify { value in
+						if let newValue = event.value {
+							value = newValue
+						}
 					}
+					observer.action(event)
 				}
 			}
 		}
@@ -675,31 +678,28 @@ public final class MutableProperty<Value>: ComposableMutablePropertyProtocol {
 	/// Atomically modifies the variable.
 	///
 	/// - parameters:
-	///   - action: A closure that accepts old property value and returns a new
-	///             property value.
+	///   - action: A closure that accepts an inout reference to the value.
 	///
 	/// - returns: The result of the action.
 	@discardableResult
 	public func modify<Result>(_ action: (inout Value) throws -> Result) rethrows -> Result {
-		return try box.modify(didSet: { self.observer.send(value: $0) }) { value in
-			return try action(&value)
+		return try box.begin { storage in
+			defer { observer.send(value: storage.value) }
+			return try storage.modify(action)
 		}
 	}
 
 	/// Atomically modifies the variable.
 	///
+	/// - warning: The reference should not be escaped.
+	///
 	/// - parameters:
-	///   - didSet: A closure that is invoked after `action` returns and the value is
-	///             committed to the storage, but before `modify` releases the lock.
-	///   - action: A closure that accepts old property value and returns a new
-	///             property value.
+	///   - action: A closure that accepts a reference to the property storage.
 	///
 	/// - returns: The result of the action.
 	@discardableResult
-	internal func modify<Result>(didSet: () -> Void, _ action: (inout Value) throws -> Result) rethrows -> Result {
-		return try box.modify(didSet: { self.observer.send(value: $0); didSet() }) { value in
-			return try action(&value)
-		}
+	internal func begin<Result>(_ action: (PropertyStorage<Value>) throws -> Result) rethrows -> Result {
+		return try box.begin(action)
 	}
 
 	/// Atomically performs an arbitrary action using the current value of the
@@ -719,16 +719,40 @@ public final class MutableProperty<Value>: ComposableMutablePropertyProtocol {
 	}
 }
 
+internal struct PropertyStorage<Value> {
+	private unowned let box: PropertyBox<Value>
+
+	var value: Value {
+		return box._value
+	}
+
+	func modify<Result>(_ action: (inout Value) throws -> Result) rethrows -> Result {
+		guard !box.isModifying else { fatalError("Nested modifications violate exclusivity of access.") }
+		box.isModifying = true
+		defer { box.isModifying = false }
+		return try action(&box._value)
+	}
+
+	fileprivate init(_ box: PropertyBox<Value>) {
+		self.box = box
+	}
+}
+
 /// A reference counted box which holds a recursive lock and a value storage.
 ///
 /// The requirement of a `Value?` storage from composed properties prevents further
 /// implementation sharing with `MutableProperty`.
 private final class PropertyBox<Value> {
+
 	private let lock: Lock.PthreadLock
-	private var _value: Value
-	private var isModifying = false
+	fileprivate var _value: Value
+	fileprivate var isModifying = false
 
-	var value: Value { return modify { $0 } }
+	internal var value: Value {
+		lock.lock()
+		defer { lock.unlock() }
+		return _value
+	}
 
 	init(_ value: Value) {
 		_value = value
@@ -741,11 +765,9 @@ private final class PropertyBox<Value> {
 		return try action(_value)
 	}
 
-	func modify<Result>(didSet: (Value) -> Void = { _ in }, _ action: (inout Value) throws -> Result) rethrows -> Result {
+	func begin<Result>(_ action: (PropertyStorage<Value>) throws -> Result) rethrows -> Result {
 		lock.lock()
-		guard !isModifying else { fatalError("Nested modifications violate exclusivity of access.") }
-		isModifying = true
-		defer { isModifying = false; didSet(_value); lock.unlock() }
-		return try action(&_value)
+		defer { lock.unlock() }
+		return try action(PropertyStorage(self))
 	}
 }

Tests/ReactiveSwiftTests/ActionSpec.swift

@@ -131,6 +131,39 @@ class ActionSpec: QuickSpec {
 				expect(action.isExecuting.value) == false
 			}
 
+			it("should not deadlock when its enabled state affects its state property without constituting a feedback loop") {
+				// Emulate control binding: When a UITextField is the first responder and
+				// is being disabled by an `Action`, the control events emitted might
+				// feedback into the availability of the `Action` synchronously, e.g.
+				// via a `MutableProperty` or `ValidatingProperty`.
+				var isFirstResponder = false
+
+				action.isEnabled.producer
+					.filterMap { isActionEnabled in !isActionEnabled && isFirstResponder ? () : nil }
+					.startWithValues { _ in enabled.value = false }
+
+				enabled.value = true
+				expect(enabled.value) == true
+				expect(action.isEnabled.value) == true
+				expect(action.isExecuting.value) == false
+
+				isFirstResponder = true
+				let disposable = action.apply(0).start()
+				expect(enabled.value) == false
+				expect(action.isEnabled.value) == false
+				expect(action.isExecuting.value) == true
+
+				disposable.dispose()
+				expect(enabled.value) == false
+				expect(action.isEnabled.value) == false
+				expect(action.isExecuting.value) == false
+
+				enabled.value = true
+				expect(enabled.value) == true
+				expect(action.isEnabled.value) == true
+				expect(action.isExecuting.value) == false
+			}
+
 			it("should not deadlock") {
 				final class ViewModel {
 					let action2 = Action<(), (), NoError> { _ in SignalProducer(value: ()) }