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

Synchronous Unsubscribe #266

Closed
benlesh opened this issue Sep 4, 2015 · 15 comments
Closed

Synchronous Unsubscribe #266

benlesh opened this issue Sep 4, 2015 · 15 comments

Comments

@benlesh
Copy link
Member

benlesh commented Sep 4, 2015

The Proposal

So currently we don't support synchronous unsubscription because of our API. What has been/is being proposed is a RxJava-style solution (not with backpressure, just with the start handler):

It looks like this:

Observer interface changes to:

interface Observer {
  next(value: T): void;
  error(err: any): void;
  complete(): void;
  start(subscriber: Subscriber): void;
}

All this means is that the execution of a synchronous function will get access to an unsubscribe method as soon as subscription occurs.

Explanation of "Synchronous Unsubscribe"

Consider the following with the current version of RxJS Next:

var subscription = Observable.of(1).subscribe(x => {
  console.log(subscription); // undefined
});
console.log(subscription.isUnsubscribed); // true

If you subscribe to a synchronous Observable, the returned subscription object is pointless, because it's always going to be unsubscribed by the time you hit the next line in your current block of code.

Now if you were able to get access to the Subscriber (which has all Subscription logic on it) in a start method at subscription time:

var subscription = null;
Observable.of(1).subscribe(function next(x) {
  console.log(subscription.isUnsubscribed); //false
  /* you could call `subscription.unsubscribe()` here if you wanted */
}, 
null, // error
null, // complete
function start(subscriber) {
  subscription = subscriber;
});

console.log(subscription.isUnsubscribed); //true

Unanswered Questions

  • In what scenarios is this useful? Are they valid scenarios (should we encourage people to avoid them somehow)?
  • Will this detract from performance?
  • Will this be at all useful in making existing operators (take for example) more performant?

Other Things

If/whenever an RxFlowable library (@benjchristensen) is developed for JS, it will be essentially a mirror of this library geared for server/node use with the addition of backpressure. It will most likely have a start() handler on it, because the start handler is where you initialize backpressure by requesting the initial values. Adding this to our API may help people jump between the two types.

@zenparsing has demonstrated how this API can make operators potentially very ugly to implement. He has also discussed this with @jhusain at length. It's doubtful that this API will make the es7 observable spec, which is a corner goal for this library.

/cc @headinthebox @mattpodwysocki

@benlesh
Copy link
Member Author

benlesh commented Sep 4, 2015

Oh... additional question: Would it be possible to add this to our API without changing much about our existing operator implementations? My gut tells me "yes", but I haven't investigated this throughly.

also (+@trxcllnt)

@headinthebox
Copy link

Just my 2 cents, I never felt the need to access the subscription inside subscribe, so I am not sure why we want to complicate the API with that.

@benlesh
Copy link
Member Author

benlesh commented Sep 4, 2015

Your 2 cents is greatly appreciated, @headinthebox. Can you elaborate as to why you don't feel the need to access the subscription inside subscribe?

My gut tells me that without the need for RxJava-style backpressure, it's probably a special Rx flavor of code-smell to access subscription inside of the subscribe function. But I don't know what I don't know (hence this thread).

@headinthebox
Copy link

It is a huge code smell, since you are probably doing something (stateful) that you should do with an operator. If you really want to unsubscribe from within say the next function, you can always use a takeUntil and a subject and trigger that, so you do not gain any additional expressive power.

@benlesh
Copy link
Member Author

benlesh commented Sep 4, 2015

What do you want to do with it?

Unknown. It's been brought up by @jhusain and @trxcllnt many times, and I wanted to have an issue to discuss and track it. In a recent hallway conversation with @jhusain, I was told @zenparsing refuted the need for this API really well. I'd like to capture some of that.

@trxcllnt
Copy link
Member

trxcllnt commented Sep 5, 2015

@headinthebox takeUntil won't help in the scenario that the source Observable synchronously onNext's events (as the current implementation does by default). Presently the only way to unsubscribe from inside next is to subscribe with callbacks (they get attached to the initial Subscriber), and call this.unsubscribe() which is more of a bug than a feature.

If Observer stored the Subscription injected into start, Observer could implement its own unsubscribe method to synchronously unsubscribe from within onNext.

@zenparsing
Copy link

In my opinion, sending data to the observer before subscribe has returned to the caller is an anti-pattern for JS. That's why I would like Observable.of and Observable.from to send their data in a future job.

Let's call sending data to the observer before subscribe has returned "eager delivery". Eager delivery is not required in order to model event listeners or Subjects, and I haven't found any killer application for it. It's a corner case, as far as I can tell. It's also confusing to programmers (read "Zalgo"). I'm happy that we can support it, but I'm very much against making the core abstraction more complex in order to provide additional support for eager delivery.

Moreover, Rx has a long history and the lack of "synchronous unsubscription" does not seem to have resulted in any gaping functional holes (as far as I can tell).

Our perspectives are a little different. I'm thinking in terms of the JS standard library, and for the standard library, there's a seeming paradox at play: the simpler the abstraction is, the more broadly useful it is.

Consequently, I'm in favor of keeping Observables as simple as possible (more simple than Rx, even), not making them more complex.

Moreover, I don't think we should have a RxJava-style request(n) API as a targeted future state for Observables in JS. I think that distracts us from creating a really solid and simple API that is maximally-useful to the broadest swath of programmers.

@jhusain
Copy link

jhusain commented Sep 5, 2015

I've tried and failed to come up with a good example of why synchronous unsubscription is useful. At this point I'm inclined to agree with Kevin unless someone can come up with really good reason for synchronous unsubscription.

JH

On Sep 4, 2015, at 6:52 PM, zenparsing notifications@github.com wrote:

In my opinion, sending data to the observer before subscribe has returned to the caller is an anti-pattern for JS. That's why I would like Observable.of and Observable.from to send their data in a future job.

Let's call sending data to the observer before subscribe has returned "eager delivery". Eager delivery is not required in order to model event listeners or Subjects, and I haven't found any killer application for it. It's a corner case, as far as I can tell. It's also confusing to programmers (read "Zalgo"). I'm happy that we can support it, but I'm very much against making the core abstraction more complex in order to provide additional support for eager delivery.

Moreover, Rx has a long history and the lack of "synchronous unsubscription" does not seem to have resulted in any gaping functional holes (as far as I can tell).

Our perspectives are a little different. I'm thinking in terms of the JS standard library, and for the standard library, there's a seeming paradox at play: the simpler the abstraction is, the more broadly useful it is.

Consequently, I'm in favor of keeping Observables as simple as possible (more simple than Rx, even), not making them more complex.

Moreover, I don't think we should have a RxJava-style request(n) API as a targeted future state for Observables in JS. I think that distracts us from creating a really solid and simple API that is maximally-useful to the broadest swath of programmers.


Reply to this email directly or view it on GitHub.

@headinthebox
Copy link

We seem to talk a lot about sources that synchronously send events, but in practice, in my experience, you only do that for "demos", just to have some quick and dirty sample input. If a source is really synchronous you are better off using an iterable instead, and it is kind of stupid to make it "push". But that is another discussion.

Say you cannot use takeUntil, and you really want to access the subscription, you can always wrap an Observable.create around the source, and pick it up it there.

Most importantly however, not that if you pass in the handlers separately, there is no way you can get at the subscriber passed into start in say next, so you have to assign it to a global variable anyway, making it the same as the second example above where you assign it to null first (perhaps it is telling there was no example using the proposed chance).

@benlesh
Copy link
Member Author

benlesh commented Sep 18, 2015

I'm going to close this discussion for now. It seems like we can't come up with a valid use case to alter the current design/architecture. At the very least, this discussion has died off, and I can close this issue.

... this does not, of course, mean that we won't consider making the proposed changes should we identify a valid need or reason to do so.

@benlesh benlesh closed this as completed Sep 18, 2015
@trxcllnt
Copy link
Member

@Blesh @jhusain would we need this if we were to successfully model EventTarget as an Observable?

@benlesh
Copy link
Member Author

benlesh commented Sep 18, 2015

cc @zenparsing ^^^

@zenparsing
Copy link

No, because calling addEventListener will never result in an event being dispatched to the listener before it returns.

@trxcllnt
Copy link
Member

I'm mostly thinking of custom EventTarget implementations. Is there anything specified that prohibits someone from implementing an addEventListener that synchronously dispatches an event (say, its latest value) to new listeners?

@zenparsing
Copy link

Someone could write their own EventTarget-ish thing that does that, but they shouldn't. : )

I just looked up Node's EventEmiiter and it does in fact fire an event when addListener is called.

https://github.com/nodejs/node/blob/master/lib/events.js#L204

However, it will never actually execute the provided callback before returning.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants