Skip to content

Improve Signal state management to avoid an ARC race. #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 17, 2017
Merged

Improve Signal state management to avoid an ARC race. #456

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1ad2524
Be conservative in locking in `Signal.Core.send` for now.
andersio Jun 15, 2017
557979e
Remove `AliveState` and `TerminatingState`.
andersio Jun 15, 2017
a0e7f74
Renamed `updateLock` to `stateLock`. Updated the documentation.
andersio Jun 16, 2017
a407b6e
Clean `tryToCommitTermination()` up.
andersio Jun 16, 2017
ab84a10
Revert removal of terminating check fast path. Remove OperationResult.
andersio Jun 16, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 46 additions & 62 deletions Sources/Signal.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,11 @@ public final class Signal<Value, Error: Swift.Error> {
private let core: Core

private final class Core {
private enum OperationResult {
case shouldDispose
case shouldTryCommitTermination
case none
}

/// The disposable associated with the signal.
///
/// Disposing of `disposable` is assumed to remove the generator
/// observer from its attached `Signal`, so that the generator observer
/// as the last +1 retain of the `Signal` core may deinitialize.
private let disposable: SerialDisposable

/// The state of the signal.
Expand Down Expand Up @@ -104,6 +102,8 @@ public final class Signal<Value, Error: Swift.Error> {
} else {
self.stateLock.unlock()
}

tryToCommitTermination()
} else {
self.sendLock.lock()
self.stateLock.lock()
Expand All @@ -119,20 +119,28 @@ public final class Signal<Value, Error: Swift.Error> {
}

self.sendLock.unlock()
}

// Check if the status has been bumped to `terminating` due to a
// terminal event being sent concurrently or recursively.
//
// The check is deliberately made outside of the `sendLock` so that it
// covers also any potential concurrent terminal event in one shot.
//
// Related PR:
// https://github.com/ReactiveCocoa/ReactiveSwift/pull/112
if .shouldDispose == self.tryToCommitTermination() {
// Dispose only after notifying observers, so disposal
// logic is consistently the last thing to run.
self.disposable.dispose()
// Check if the status has been bumped to `terminating` due to a
// terminal event being sent concurrently or recursively.
//
// The check is deliberately made outside of the `sendLock` so that it
// covers also any potential concurrent terminal event in one shot.
//
// Related PR:
// https://github.com/ReactiveCocoa/ReactiveSwift/pull/112
//
// While calling `tryToCommitTermination` is sufficient, this is a fast
// path for the recurring value delivery.
//
// Note that this cannot be `try` since any concurrent observer bag
// manipulation might then cause the terminating state being missed.
stateLock.lock()
if case .terminating = state {
Copy link
Member Author

@andersio andersio Jun 16, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is resurrected because acquiring the spinlock is consistently & considerably faster than calling tryToCommitTermination() on my machine (>5% per observer callout). 🤦‍♂️

stateLock.unlock()
tryToCommitTermination()
} else {
stateLock.unlock()
}
}
}

Expand Down Expand Up @@ -178,27 +186,12 @@ public final class Signal<Value, Error: Swift.Error> {
let observer = newObservers.remove(using: token)
self.state = .alive(newObservers, hasDeinitialized: hasDeinitialized)

var result = OperationResult.none

// Ensure `observer` is deallocated after `stateLock` is
// released to avoid deadlocks.
withExtendedLifetime(observer) {
// Start the disposal of the `Signal` core if the `Signal` has
// deinitialized and there is no active observer.
result = tryToDisposeSilentlyIfQualified()

stateLock.unlock()
}

if result == .shouldTryCommitTermination {
result = tryToCommitTermination()
}

if result == .shouldDispose {
// Disposing of `disposable` is assumed to remove the generator
// observer from its attached `Signal`, so that the generator observer
// as the last +1 retain of the `Signal` core may deinitialize.
disposable.dispose()
tryToDisposeSilentlyIfQualified(unlocking: stateLock)
}
} else {
stateLock.unlock()
Expand All @@ -212,9 +205,7 @@ public final class Signal<Value, Error: Swift.Error> {
/// method as a result of a false positive `terminating` check is permitted.
///
/// - precondition: `stateLock` must not be acquired by the caller.
///
/// - returns: `.shouldDispose` if the attempt succeeds. `.none` otherwise.
private func tryToCommitTermination() -> OperationResult {
private func tryToCommitTermination() {
// Acquire `stateLock`. If the termination has still not yet been
// handled, take it over and bump the status to `terminated`.
stateLock.lock()
Expand All @@ -226,20 +217,19 @@ public final class Signal<Value, Error: Swift.Error> {
state = .terminated
stateLock.unlock()

defer { sendLock.unlock() }

if let event = terminationKind.materialize() {
for observer in observers {
observer.action(event)
}
}

return .shouldDispose
sendLock.unlock()
disposable.dispose()
return
}
}

stateLock.unlock()
return .none
}

/// Try to dispose of the signal silently if the `Signal` has deinitialized and
Expand All @@ -250,27 +240,31 @@ public final class Signal<Value, Error: Swift.Error> {
///
/// - precondition: `stateLock` must have been acquired by the caller.
///
/// - returns: `.shouldDispose` if the signal has terminated immediately.
/// `.shouldTryCommitTermination` if the signal has transitioned to
/// `terminating` state, and the caller should try to commit the
/// termination with `tryToCommitTermination`. `.none` otherwise.
private func tryToDisposeSilentlyIfQualified() -> OperationResult {
/// - parameters:
/// - stateLock: The `stateLock` acquired by the caller.
private func tryToDisposeSilentlyIfQualified(unlocking stateLock: Lock) {
assert(!stateLock.try(), "Calling `unconditionallyTerminate` without acquiring `stateLock`.")

if case let .alive(observers, true) = state, observers.isEmpty {
// Transition to `terminated` directly only if there is no event delivery
// on going.
if sendLock.try() {
defer { sendLock.unlock() }
self.state = .terminated
return .shouldDispose
stateLock.unlock()
sendLock.unlock()

disposable.dispose()
return
}

self.state = .terminating(Bag(), .silent)
return .shouldTryCommitTermination
stateLock.unlock()

tryToCommitTermination()
return
}

return .none
stateLock.unlock()
}

/// Acknowledge the deinitialization of the `Signal`.
Expand All @@ -283,17 +277,7 @@ public final class Signal<Value, Error: Swift.Error> {
}

// Attempt to start the disposal of the signal if it has no active observer.
var result = tryToDisposeSilentlyIfQualified()

stateLock.unlock()

if result == .shouldTryCommitTermination {
result = tryToCommitTermination()
}

if result == .shouldDispose {
disposable.dispose()
}
tryToDisposeSilentlyIfQualified(unlocking: stateLock)
}

deinit {
Expand Down