Skip to content

Commit

Permalink
Fixed a race condition in the Signal.observe.
Browse files Browse the repository at this point in the history
An explicit guard is added to 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.
  • Loading branch information
andersio committed Feb 17, 2017
1 parent 7920f19 commit 2dfa7cb
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 4 deletions.
17 changes: 14 additions & 3 deletions Sources/Signal.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
17 changes: 16 additions & 1 deletion Tests/ReactiveSwiftTests/ActionSpec.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit 2dfa7cb

Please sign in to comment.