-
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
Lifetime
-based producer resource management
#334
Conversation
SignalProducer
resource managementCompositeDisposable
and SignalProducer
resource management
CompositeDisposable
and SignalProducer
resource managementDisposableCollector
interface for SignalProducer
.
0a42d67
to
5d01764
Compare
I definitely think it makes sense to close this hole. I like that you split out #363—that will make this diff more readable. I don't love the name Would it make sense to pass an |
|
5d01764
to
96a8b33
Compare
|
But they aren't removable inside the block? |
|
🤦♂️ |
I guess I'd lean towards |
|
The |
I think I'm 👍 with |
96a8b33
to
3614fc9
Compare
DisposableCollector
interface for SignalProducer
.Lifetime
-based producer resource management
3614fc9
to
b68c1e0
Compare
|
Sources/Lifetime.swift
Outdated
return (Lifetime(token), token) | ||
internal enum Backing { | ||
case token(ended: Signal<(), NoError>) | ||
case disposable(CompositeDisposable) |
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.
Do we need to support two different backings?
It's only used in one spot and it could be emulated by holding a reference to the token in an ActionDisposable
.
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 can't see how ActionDisposable
can emulate it. But I think it can be backed by just CompositeDisposable
.
@ReactiveCocoa/reactiveswift Any opinions on the passing a |
6ff11ac
to
92dccf5
Compare
A side note: we may unify let signal = Signal { observer, lifetime in
let disposable = self.observe { ... }
if let d = disposable { lifetime.observeEnded(d) }
} |
92dccf5
to
8fab9ad
Compare
It seems like this change has broken the ability to interrupt the running RequestProducer via disposing of the Disposable, in a way that original let mySignalProducer = SignalProducer<Void, NoError> { observer, lifetime in
lifetime.observeEnded {
print("> lifetime ended")
}
async ... {
print("> completed")
observer.sendCompleted()
}
}
let disposable = mySignalProducer.start()
print("> started")
disposable.dispose()
print("> disposed") So the log for the example above would look like this:
So the lifetime inside the ResponseProducer's init closure remains alive until Producer completes. I should mention this worked differently in ReactiveSwift v1.*, where SignalProducer would be interrupted immediately upon disposing... |
The producer doesn't get to complete. It is interrupted immediately. The problem here is that the piece of async work is always going to execute, because it is not bound to the
A slightly tweaked snippet to illustrate this: let mySignalProducer = SignalProducer<Void, NoError> { observer, lifetime in
lifetime.observeEnded {
print("> lifetime ended")
}
// MARK: A
QueueScheduler.main.schedule {
// MARK: B
// lifetime += QueueScheduler.main.schedule {
print("> complete?")
observer.sendCompleted()
}
}
let disposable = mySignalProducer
.logEvents()
.start()
disposable.dispose() Output:
Now comment A and uncomment B. |
@andersio thank you for the explanation, now I understand it better. Is that possible to bound the piece of work to the lifetime without appealing to closure in a scheduler (basically how to do the same but syncronously)? It looks like unless I bound other disposable to the lifetime it'll disregard the fact of disposing as |
|
Yep, that totally makes sense. Thank you |
If you wish to be able to cancel it immediately and synchronously, you would have to adopt operators like e.g. Try to apply |
Note that this also means that the decision to cancel has to be made before |
In RAS 1.0 and earlier, the start handler is passed a
CompositeDisposable
directly. While it is not an intended way to interrupt the producer, disposing of it can often, but not always, yield similar results as sending terminal events. It generally works only if the producedSignal
has an upstream to be interrupted in the first place.Related test case: 5d01764#diff-9cd786f9e52e786771d96cf8c42d3befL34
The PR proposes to close this unreliable semantic hole by using
Lifetime
instead.Alternative
The start handler may instead follow
Signal.init
to be(Observer<Value, Error>) -> Disposable?
. While it achieves the same goal without requiring a new type, this however cannot be migrated due to tuple splat messing with overloading.Possible follow-up
If this gets accepted, we may consider matching the generator closure of
Signal.init
with this for consistency. However, like the alternative above, such change cannot be migrated. That said it is expected to have less impact, sinceSignal.pipe
is the more popular API whileSignal.init
is primarily used by operators.