-
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
Goodbye interrupted
...?
#144
Comments
I'm pretty busy these days and honestly can't think about the specifics in detail, but I'm 100% happy to see this implementation detail go away and simplify the API :) especially since most of us (I at least) were never confident with its nuances. |
If you cancel a
|
Hmm, you raise a valid point for observers relying on a terminal event to clean up resources. In the proposal, no events would be sent, because no work that yields event has been committed. The same also applies to observing a terminated
Edit: Other alternatives that could be a step backward:
Edit: Though having an |
interrupted
.interrupted
is staying.)
Regardless of removing it or not, I think it would still be nice to refactor it by establishing an explicit reversed channel, which can be reused for #163. Then ideally we'd still achieve the goal on one less I have a wild idea on how to hide it completely from the public API, but I haven't got the time to iterate on it yet. |
interrupted
is staying.)interrupted
.
It seems making case .interrupted:
// cleanup
case let .failed(error):
// handle error
// cleanup to: case let .failure(context):
if let error = context.error {
// handle error
}
// context.isInterrupted: Bool
// cleanup |
That doesn't seem very user friendly to me. The current API seems much better—at least for our users. |
Have the same feel after skimming through the codebase again. Anyway, the refactoring gonna be done in #163. |
Goodbye, our beloved
interrupted
.TL;DR
By removing, we would have:
Signal
to be created when starting aSignalProducer
.Composed properties would benefit significantly - in particular the use of
replayLazily
- in the same way as Less hops in operator lifting. #140 onSignalProducer
.Rationale
Back in
ReactiveSwift -2.0ReactiveCocoa 3.0,Signal
has a different lifetime semantic. That is, holding the event emitter of aSignal
would indefinitely retain theSignal
unless any terminal event is sent. Sointerrupted
was introduced to helpSignalProducer
interrupt the leaves of itsSignal
graph (the ultimate upstreams) without hassle.In ReactiveSwift 1.0, a new
Signal
lifetime semantic was introduced. Let's call it Automatic Observer and Reference Counting (AORC), which is pretty predictable in when aSignal
would dispose of itself:Alright. So why does it matter?
Look closer to the conditions, and the event emitter is off the list. That means for a
Signal
graphtigerMom.map(a).map(b)
, the two transformedSignal
s are not unilaterally forced to live as long as the ultimate upstream of theirs, i.e.tigerMom
. They can be disposed of, as long as the two AORC conditions are met. At that point, they would also cease their observations to the upstreamSignal
.Okay. But... how is it relevant to
interrupted
?We just mentioned that a
Signal
can live on its own, and ceases its observation to an upstream when disposed. Doesn't this sound like whatinterrupted
is designed for?Now, the most important part is that the
Signal
graph created by aSignalProducer
generally comprises of only one-observerSignal
nodes (except the root and any multitcasting done by a custom operator throughstartWithSignal
). That means if the root node is disposed of, the disposal would definitely propagate along the graph to the leaves.(Well, unless some of our operators being naughty.)
It is exactly what
interrupted
does for us, isn't it? Interrupting one level by one level, from the root node to the leaves.Note that the existing
startWithSignal
The current
startWithSignal
, should it remains unchanged, would break the assumptions that the removal ofinterrupted
relies on. Specifically, it permits multicasting and grabbing an external reference to the producedSignal
. The migration plan below suggests introducing a internal "unsafe" variant of it to circumvent the issue.The Migration Plan
The proposal here is to remove the
interrupted
event. But since it would not make in 1.0, it should be staged out.Phase 1: Internal Refactoring @ 1.x
SignalProducer
to depend on the disposal of its producedSignal
;interrupted
;interrupted
; andstartWithUnicastSignal
.startWithUnicastSignal
assumes that it would ever have at most one observer, and theSignal
reference would not escape thesetUp
closure scope. We can ensure these at runtime with assertions on the observer count andisKnownUniquelyReferenced
. But it may not be ideal to be part of the public API. Anyhow, it should be able to replace most use ofstartWithSignal
in our codebase, including all thestart(.+?)
methods.startWithSignal
would be rebuilt on top ofstartWithUnicastSignal
without changing its behaviour.Since the
interrupted
event cannot be removed,SignalProducer
still has to emit the event for compatibility. To achieve this, anySignal
s created bySignalProducer
would have to emit aninterrupted
event when disposed without a terminal event. It can be done by a special flag inSignal
that would be consumed by the deinitializer.Notes on the new start handler format (#140).
The new internal start handler format would no longer include an interrupter (cancel disposable) in its returning tuple.
Phase 2: Deprecation @ 1.x
Deprecate
sendInterrupted
,observeInterrupted
,startWithInterrupted
andObserver(interrupted:)
in the public API.As the internal refactoring has been done, our codebase should be free from deprecation warnings.
Phase 3: Removal @ 2.0
Remove
Event.interrupted
, allinterrupted
related APIs and test cases, and the workaround inSignal
.The text was updated successfully, but these errors were encountered: