-
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
Fixed a scenario of downstream interruptions being dropped. #577
Conversation
77ac5c6
to
2bf6094
Compare
7564d1d
to
02d76cc
Compare
02d76cc
to
d0a0666
Compare
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.
This should also be added to the changelog.
Sources/Observer.swift
Outdated
/// - observer: The observer to transform. | ||
/// - transform: The transform. | ||
/// - disposables: The disposable to be disposed of upon termination. | ||
/// Used by `SignalProducer` only, since `Signal` takes. |
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.
This init
doesn't have a disposables
parameter. The line also seems incomplete.
@mdiep I've inlined the now diverged code paths in |
c01d0f0
to
755a896
Compare
755a896
to
f8243e4
Compare
Scope
The issue apparently exists since 2.0 with producer lifting optimisation (#140), and is also applicable to
TransformerCore
.Symptom
With a terminated upstream, manual interruption of time shifted producers, including
delay
,observe(on:)
,throttle
,debounce
andlazyMap
, would be ignored.This is due to the operator lifting scheme introduced in #140, which blatantly assumes the upstream always lives longer than the downstream & therefore is always available to handle the downstream/user interruption.
Simple example:
Proposed Fix
Transformation Core
The first part of the fix establishes a
CompositeDisposable
specifically for the event transformation (the combined version if nested), which is disposed of only if the transformation outputs a terminal event.(Pretty much like operator lifting in 1.0 and earlier, but one
CompositeDisposable
being shared by all operators).This allows asynchronous event transformation to response to user/downstream interruption properly.
Operator Lifting for
SignalProducer
The second part of the fix reverts the implementation of
SignalProducer.lift
to pre-#140 times.Operator lifting has a significantly reduced role since 3.0. With a large set of operators already being lowered as event transforms, I believe the impact would be minimal.
Note on async operators &
interrupted
deliveryThe fix still maintains the semantic of
interrupted
respecting async operators, introduced in 2.0 (#140).Summary: When the product of async operators like
observe(on:)
gets interrupted, unless otherwise specified, theinterrupted
event should be delivered on the respective scheduler.lazyMap
does not follow this yet, and we should probably reimplement it as event transform at a later point.timeout
is an odd one with only the failure delivered asynchronously, so it isn't bound by the said semantic.Acknowledgement
Special thanks to Takeru Chuganji (@chuganzy) raising this on Slack.
Checklist