-
Notifications
You must be signed in to change notification settings - Fork 429
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.
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
Revisit changes to the Signal
lifetime semantics to improve tooling interoperability.
#355
Conversation
5beb43f
to
df7f83c
Compare
expect(disposable.isDisposed) == true | ||
} | ||
|
||
it("should dispose of the returned disposable upon interrupted") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failure
e825a09
to
e47b6d6
Compare
I'll have to think about the impact of this.
Can you explain this a bit more? I haven't looked over the code, but I'm not sure when this would happen. |
The rule of active observers keeping the
This change does not affect operators, since they have their generator observer retained by the upstream signals. Only the explicit, non-composition uses of Say we have a pipe and we have observed it: func scope() {
let (signal, observer) = Signal.pipe()
signal.observe { _ in }
} In 1.x, the self reference asserted by the sole active observer would keep the With this PR, the We may consider emitting |
e47b6d6
to
8d9e128
Compare
Can you add the external observers to your diagram above? I think that's the tricky piece to think about. Would it work to replace the self reference with an actual strong reference from the observer to the signal? That seems like it would maintain the existing semantics (which are conceptually simpler IMO) but also fix the tooling issues.
I think it's preferable to not have this be the case, which is why I ask. But if this the only way to improve tooling interop, then I think it's worth it. |
It does not solve the issue of deliberate retain cycle being caught with the current implementation. Specifically, if an observer now holds a strong reference to the
|
A side note: I have tried many tricks, hoping to trick the memory debugger not to recognise the retain cycle. But none of these works.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Finally wrapped my head around this.
A few requests that I think will improve clarity, but overall the approach seems 👍.
Sources/Signal.swift
Outdated
signal.state = .terminated | ||
signal.updateLock.unlock() | ||
/// Used to indicate if the `Signal` shell has deinitialized. | ||
private var shellDeinitialized: Bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should use signal
instead of shell
here and in the related methods. I think that's clearer.
Sources/Signal.swift
Outdated
withExtendedLifetime(observer) { | ||
// Dispose of the `Signal` core if the shell has deinitialized | ||
// and it has no active observer. | ||
result = tryTerminateSilently(if: state.observers.isEmpty && shellDeinitialized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition (state.observer.isEmpty && shellDeinitialized
) should move into tryTerminateSilently
. It's the always the same logic. (In the other case, we know that shellDeinitialized
is true
, but it doesn't hurt to explicitly check it.)
Sources/Signal.swift
Outdated
// released to avoid deadlocks. | ||
withExtendedLifetime(observer) { | ||
// Dispose of the `Signal` core if the shell has deinitialized | ||
// and it has no active observer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify that this comment only starts the disposal?
Sources/Signal.swift
Outdated
observer.sendCompleted() | ||
return nil | ||
if result == .shouldDispose { | ||
disposable.dispose() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a comment here clarifying that this completes the disposal by removing the last strong reference to the core?
Sources/Signal.swift
Outdated
return .none | ||
} | ||
|
||
defer { _sendLock?.unlock() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this a bit confusing as written. I think we should go with a more straightforward version that's self explanatory:
if sendLock == nil {
if !self.sendLock.try() {
return .none
}
defer { self.sendLock.unlock() }
}
Sources/Signal.swift
Outdated
/// `tryTerminate` would attempt to acquire the `sendLock`. | ||
/// | ||
/// - returns: `.shouldDispose` if the attempt succeeds. `.none` otherwise. | ||
private func tryCommitTermination(acquired sendLock: Lock? = nil) -> OperationResult { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add to after try in this method and the one below? tryToCommitTermination
Sources/Signal.swift
Outdated
} | ||
shellDeinitialized = false | ||
disposable = SerialDisposable() | ||
disposable.inner = generator(Observer(self.send)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment here to highlight that it's the other +1?
That test failure looks like it might be legit. |
|
Rationale
Cease the self reference in
AliveState
which causes theSignal
and all its retained objects being reported as leaks, so that ReactiveSwift would work better with the memory debuggers.Details
This PR adapts the original design in ReactiveCocoa/ReactiveCocoa#2959.
To recap, the rule of automatic disposal is having no observer and having no external retain. This PR does not change this principle, but makes a few changes in the implementation:
A signal is now broken into a public shell
Signal
and an internal coreSignal.Core
.The shell is what the public users retain and interact with. The shell is neither strongly or weakly referenced by the core, the generator disposable or any disposable created.
The generator observer now holds a strong reference to
Signal.Core
, and pipes events toSignal.Core.send(_:)
. Previously, it holds a weak reference toSignal
.Here is the ownership graph from the perspective of an operator. Note that there is no circular strong reference in the graph as we had before.
These changes consequently lead to a few boundary cases that is not source compatible with ReactiveSwift 1.0:
A
nil
weak reference to a signal no longer means that the signal has been disposed of.on(disposed:)
would become the only mean to assert this.If the generator observer deinitialises, it now triggers the automatic disposal since no event can be sent to the
Signal
from this point of time. Previously, it has no side-effect.In other words, the rule of automatic disposal is now expanded to be (1) having no observer and having no external retain; OR (2) having the generator observer deinitialized".
These are not expected to be of huge impact.
Implementation
The threading design and the logic of event delivery is generally untouched.
The implementation in
Signal
is moved toSignal.Core
.Signal
now only forwards calls toSignal.Core
.Signal.Core.init
now disposed of the generator disposable immediately, if the signal is terminated within the generator. This was the correct and the original behavior, but had not been covered by the test cases and had been removed by previous changes. (Bugfix)tryTerminate
is now promoted to beSignal.Core.tryCommitTermination
.Signal.Core.tryTerminateSilently
is introduced for automatic disposal. Calling it guarantees theSignal
to be eventually terminated safely, but the termination may not happen immediately if there is an on-going event delivery.Signal.Core.shellDidDeinitialize
is introduced for theSignal
shell to inform theSignal
core of its deinitialization.Alternatives considered
Replace the explicit self reference with manual retains and releases via
Unmanaged
. This did not work as expected — the memory debuggers still report leaks.Status quo, which was the alternative of this more complicated design in [5.0] Changes to the lifetime semantic of
Signal
. ReactiveCocoa#2959.Testing
This has been tested with a very barebone single UITableView project that had a tremendous amount of false positives from the memory debugger when compiled with
master
. With the PR, the memory debugger reports no leak.Edit: This has also been tested with Carthage 0.22, and all test specs have passed.
Potential benefits
The design might enable an optimisation in property composition to dynamically collapse intermediate signals. Specifically in #340, it might allow the observers of the composed property's relay signal to be collapsed into the upstream signal when it is not externally retained. This is not possible at this moment due to the self reference and the semantic of
isKnownUniquelyReferenced
.