Skip to content
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

Interruption semantics for Signal are pretty tricksy #3075

Closed
andymatuschak opened this issue Jul 23, 2016 · 6 comments
Closed

Interruption semantics for Signal are pretty tricksy #3075

andymatuschak opened this issue Jul 23, 2016 · 6 comments
Milestone

Comments

@andymatuschak
Copy link
Contributor

This is probably more useful as a discussion thread for future API direction than a real bug report.

I spent a couple hours today being confused by some behavior which I ultimately boiled down to:

let (testSignal, testSignalObserver) = Signal<Int, NoError>.pipe()
let disposable = testSignal
    .map { v -> Int in
        print("Got \(v)")
        return v
    }
    .logEvents()
    .observe { _ in () }

disposable!.dispose()

testSignalObserver.sendNext(3)

The output is:

Got 3
[] Next 3 fileName: /var/folders/0s/05rqp6793jl65kl_nn7qvfpw0000gn/T/lldb/33492/playground1.swift, functionName: __lldb_expr_1, lineNumber: 32

I was surprised that the mapping block still ran when all downstream observers had been disposed. I understand now why that's happening, but it's certainly surprised us and caused a subtle bug in our app. It's interesting to note that if you're reading a signal producer's operator chain, and you see map, you do have an assurance that if all downstreams are gone, the mapping operation's not gonna run. It's often unclear when reading operator chains whether they're addressing a signal or a signal producer, so this seems like a nasty danger.

I was somewhat more surprised that there's no Interruption event emitted at any point here. I find that I don't have a very clear picture about those events' intended semantics. The docs say:

The Interrupted event indicates that the signal has terminated due to cancellation, meaning that the operation was neither successful nor unsuccessful.

"Cancellation" is an interesting word choice—it doesn't appear in the API surface or in any Signal or SignalProducer docstrings. What counts as cancellation? Clearly this sample's disposal doesn't! And clearly even experienced users have a poor understanding of this semantic. :)

I don't know what the right answer is here, other than to note that this seems pretty surprising. Maybe there should be some kind of short-circuiting for upstream Signal operators when all the downstream go away? I'm not sure I'd seriously recommend that, though: that could probably end up being just as surprising in certain circumstances.

Ultimately, we've got two API surfaces that look and feel "almost" the same, but the subtle distinctions are breeding more consequential distinctions than we'd initially intended when we imagined this split. Interesting, interesting…

/cc @NachoSoto

@andymatuschak
Copy link
Contributor Author

I'll note that this also means flatMap(.Latest) invocations where the mapping block returns a Signal are very dangerous here: they imply that many Signals are going to be created; that the author intends to only "do stuff" with the latest one; yet historically-created Signals are gonna keep running their operators' internal observers indefinitely.

@mdiep
Copy link
Contributor

mdiep commented Jul 23, 2016

I think it's pretty clear that we need to make some changes here. This behavior is surprising to basically everyone, and I don't think it's what anyone actually wants. It makes Signal pretty unusable. There have even been a few PRs for 5.0 to change this (e.g. #2959).

The only question is what the semantics should be.

The current behavior—that Signals live forever—was chosen because it would be the least surprising if you had side effects in the signal chain. But I think that's the wrong optimization.

The other options seem to be:

  1. Send interruptions all the way up the chain if a signal is manually disposed. Intermediate signals become tied to their consumers.
  2. Interrupt signals if there are no more references to them.
  3. Interrupt signals if there are no active subscribers.

I'm not sure how workable 1 and 3 are.

@andymatuschak
Copy link
Contributor Author

Thank you for that overview! Digging in on your references. :)

@andersio
Copy link
Member

andersio commented Jul 23, 2016

Option 1 doesn't seem very nice alone. In case an intermediate signal would be interrupted by any observers, it could still possibly have some other valid observers depending on it. It is also supposed to be controlled by the upstream.

Option 2 and 3 are what #2959 was about, and together they deliver the behavior of option 1. The emitter-controlled semantic is preserved, except for the moment a signal becomes "unreachable".

They also solve the issue of the lazily initialized signal on composed properties implicitly leaking if the signal is left unused.

But then signals would have to track its resources for an early disposal*, which would cause #2044 to no longer be realisable.

* under the current semantics, this is unnecessary, thus leading to #3024.

@andersio
Copy link
Member

andersio commented Jul 23, 2016

I've revived & updated #2959 for the discussion.

@mdiep
Copy link
Contributor

mdiep commented Aug 8, 2016

This has been addressed by #2959.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants