Closed andersio closed 7 years ago
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 SignalProducer
by disposing it's Disposable
, what event is sent to its observers?
Interrupted
seems useful as a way to communicate intent.
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 Signal
(it also returns a nil
tho).
Seems like we should leave the public API as is — just refactoring. Sorry @NachoSoto, it seems it gonna stay. But at least now we found it useful (?).
Edit: Other alternatives that could be a step backward:
observeDisposed
(or Lifetime
) to Signal
. Only available in startWithSignal
though.on(disposed:)
.interrupted
into failed
: Introduce ProducerError<E>
like ActionError<E>
.ProducerEvent<U, E>
with ProducerObserver<U, E>
— ehm...Edit: Though having an interrupted
event in parallel to completed
and failed
would definitely be safer for those stateful observers that assert multiple kinds of events, because the switch requires it to be handled.
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 Signal
s.
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.
It seems making SignalProducer<U, E>
take Observer<U, FailureContext<E>>
in start(_:)
would not be a bad option. We can still have all the shorthands and the type signature intact, while for those who supplies a full observer, it would be a difference from:
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 ofreplayLazily
- in the same way as #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
.