Lifetime-based producer resource management

[andersio]

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 produced Signal 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.

init(_ startHandler: @escaping (Signal<Value, Error>.Observer, Lifetime) -> Void)

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, since Signal.pipe is the more popular API while Signal.init is primarily used by operators.

[mdiep]

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 DisposableCollector though. Collector makes me think of GC, which is a a little confusing in this context. We should think of a simpler name.

Would it make sense to pass an inout [Disposable]?

[andersio]

Would it make sense to pass an inout [Disposable]?

Swift doesn't seem happy about this.

We should think of a simpler name.

Disposables? DisposableBag?

[mdiep]

DisposableSink, maybe? It's hard to come up with a good name for this.

[andersio]

Sink sounds like a point of no return, while disposables are removable in our case. Collection on the other hand sounds weird since it cannot be traversed. That's why I picked Collector.

Lifetime can be used here too (proposed originally in #143), if it makes sense to you.

The cleanup for producers does not differentiate between cancellation and termination. The collected resources are disposed of when the signal is terminated regardless how it terminates.

So it (adding a disposable to the producer disposable) is actually a synonym of observing the termination of a Signal, just without affecting the Signal's lifetime like how a normal observer would be.

[mdiep]

Sink sounds like a point of no return, while disposables are removable in our case

But they aren't removable inside the block? DisposableCollector and DisposableCollecting only have an add method. 😕

[andersio]

add returns a handle to remove them from the collection though.

[mdiep]

🤦‍♂️

[mdiep]

I guess I'd lean towards DisposableBag. It seems like the least worst option.

[andersio]

Lifetime?

[mdiep]

The += operator you mentioned in #143 would call lifetime.observeEnded(disposable.dispose)?

[andersio]

Yeah, but it isn't essential. += can be obsoleted in favour of observeEnded if we go after the Lifetime way.

I proposed adding += to Lifetime in #229, but @sharplet and you weren't fond of it.

[mdiep]

I think I'm 👍 with Lifetime, but I do still think we shouldn't add +=.

[andersio]

+= and add taking a Disposable? are added to Lifetime to assist migration. These are marked as deprecated.

[mdiep]

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.

[andersio]

I can't see how ActionDisposable can emulate it. But I think it can be backed by just CompositeDisposable.

[mdiep]

@ReactiveCocoa/reactiveswift Any opinions on the passing a Lifetime to SignalProducer.init closures instead of a Disposable? This eliminates inappropriate disposals of the disposable from inside the closure.

[andersio]

A side note: we may unify Signal.init under the same pattern.

let signal = Signal { observer, lifetime in
    let disposable = self.observe { ... }
    if let d = disposable { lifetime.observeEnded(d) }
}

[nalexn]

It seems like this change has broken the ability to interrupt the running RequestProducer via disposing of the Disposable, in a way that original CompositeDisposable would immediately propagate cancellation, while lifetime seems to not reflect the disposing of the Disposable:

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:

> started
> disposed
> completed
> lifetime ended

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...

[andersio]

@nalexn

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 Lifetime.

observer.sendCompleted in this code path is effectively a no-op, because at that point the produced Signal has been terminated due to the interruption.

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:

[] starting fileName: ReactiveSwift.playground, functionName: __lldb_expr_19, lineNumber: 34
[] started fileName: ReactiveSwift.playground, functionName: __lldb_expr_19, lineNumber: 34
[] interrupted fileName: ReactiveSwift.playground, functionName: __lldb_expr_19, lineNumber: 34
[] terminated fileName: ReactiveSwift.playground, functionName: __lldb_expr_19, lineNumber: 34
[] disposed fileName: ReactiveSwift.playground, functionName: __lldb_expr_19, lineNumber: 34
> lifetime ended
> complete?

Now comment A and uncomment B. > complete? would be gone.

[nalexn]

@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 lifetime.hasEnded is still false even after calling dispose()

[andersio]

lifetime.hasEnded being false during the course is the expected behavior. The starting side effect runs synchronously when one invokes start(), and the produced Signal is being disposed of only after start() returns.

[nalexn]

Yep, that totally makes sense. Thank you

[andersio]

If you wish to be able to cancel it immediately and synchronously, you would have to adopt operators like take(until:) (with a producer) or take(during:), which would be able to terminate the produced Signal prior to the starting side effect being triggered.

e.g. Try to apply take(until: SignalProducer(value: ())), and you should be able to observe lifetime.hasEnded being true in the starting side effect.

[andersio]

Note that this also means that the decision to cancel has to be made before start() given the synchronous scenario.