Skip to content

Commit 1205b7b

Browse files
sharpletandersio
authored andcommitted
Fix infinite feedback loop between Action states
Fixes #220. There is a potentially circular relationship between `Action.isEnabled` and `Action.isExecuting`. It's valid for users to observe properites on `Action` and feed the output back into the action state, and in turn influence `Action.isEnabled`. This resolves an issue where: 1. The user supplies a condition as the action enabled state 2. Any property derived from the internal `Action.state` is observed 3. Synchronously, the observer updates the user-supplied state property 4. Which updates the internal `state` property 5. Which causes an externally-observable event to fire, returning to (3), indefinitely. The reason for this manifesting as an infinite loop, rather than an infinite recursion, is the internal use of a `replayLazily` in `Property`: each time a side effect on the user-supplied property occurs, that queues up another event inside the replayed signal producer, and the observer is never attached because there is always a buffered event to consume. The issue was that multiple events were firing synchronously when observing `Action.isExecuting`, even though the executing state had never changed. The fix is to skip repeats on `Action.isEnabled` and `Action.isExecuting`, so that observers are only ever notified if the value actually changes, instead of whenever something triggeres a side-effect on the internal state.
1 parent 7920f19 commit 1205b7b

File tree

2 files changed

+32
-2
lines changed

2 files changed

+32
-2
lines changed

Sources/Action.swift

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -102,8 +102,8 @@ public final class Action<Input, Output, Error: Swift.Error> {
102102
}
103103
}
104104

105-
self.isEnabled = state.map { $0.isEnabled }
106-
self.isExecuting = state.map { $0.isExecuting }
105+
self.isEnabled = state.map { $0.isEnabled }.skipRepeats()
106+
self.isExecuting = state.map { $0.isExecuting }.skipRepeats()
107107
}
108108

109109
/// Initializes an action that will be conditionally enabled, and creates a

Tests/ReactiveSwiftTests/ActionSpec.swift

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,36 @@ class ActionSpec: QuickSpec {
114114
expect(action.isExecuting.value) == false
115115
}
116116

117+
if #available(macOS 10.10, *) {
118+
it("should not loop indefinitely") {
119+
let condition = MutableProperty(1)
120+
121+
let action = Action<Void, Void, NoError>(state: condition, enabledIf: { $0 == 0 }) { _ in
122+
return .empty
123+
}
124+
125+
let disposable = CompositeDisposable()
126+
127+
waitUntil(timeout: 0.01) { done in
128+
let target = DispatchQueue(label: "test target queue")
129+
let highPriority = QueueScheduler(qos: .userInitiated, targeting: target)
130+
let lowPriority = QueueScheduler(qos: .default, targeting: target)
131+
132+
disposable += action.isExecuting.producer
133+
.observe(on: highPriority)
134+
.startWithValues { _ in
135+
condition.value = 10
136+
}
137+
138+
disposable += lowPriority.schedule {
139+
done()
140+
}
141+
}
142+
143+
disposable.dispose()
144+
}
145+
}
146+
117147
describe("completed") {
118148
beforeEach {
119149
enabled.value = true

0 commit comments

Comments
 (0)