Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

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

Discussion: AbortSignal approach #5863

Closed
benlesh opened this issue Oct 28, 2020 · 7 comments
Closed

Discussion: AbortSignal approach #5863

benlesh opened this issue Oct 28, 2020 · 7 comments

Comments

@benlesh
Copy link
Member

benlesh commented Oct 28, 2020

Here I want to discuss possible approaches to improving and modernizing cancellation in RxJS with AbortSignal. I know this has been discussed elsewhere, but the goal here is to outline concrete implementation details and considerations.

Relevant Info:

  1. AbortSignal and AbortController have shipped on all major platforms (including Node).
  2. They encapsulate most of what we need to do with cancellation and signaling teardown.
  3. Providing a token that was created ahead of time resolves issues with composing synchronous "firehose" inner observables (related test: add firehose tests #5652)
  4. AbortSignal plainly lacks a few features that our current Subscription gives us:
    a. AbortSignal's method for adding and removing handlers uses addEventListener and removeEventListener which is very inconvenient.
    b. AbortSignal will not fire event handlers automatically if they're added to a signal that is already aborted.
    c. Absolutely no ergonomics for adding "child" signals to "parent" signals. (subscription1.add(subscription2) will set up behaviors, such as subscription2.unsubscribe() removing itself from subscription1).
  5. We will need a way to tie this into the existing Subscriber semantics, so we can transition to using AbortSignal smoothly.

Possible Solutions

Note that in all of these options below, the idea would be to call something like this to subscribe:

source$.subscribe(observer, signal);

And to get some sort of cancellation signal/token in the observable ctor initializer here:

const myStream$ = new Observable((subscriber, signal) => {
   // do stuff.
});

This needs to work in tandem with existing behaviors for some time. So we don't want to introduce wildly breaking changes.

1. Subclass AbortSignal (Not an option, can't be done, see comments below in thread)

Here we would create some class RxAbortSignal or the like, that had more convenient add and remove methods on it, just like Subscription does. It would also accommodate the missing behaviors we see above.

const source$ = new Observable((subscriber, signal: RxAbortSignal) => {
   const resource = new Resource();
   // custom method used here.
   signal.add(() => resource.destroy());
   resource.ondata = e => subscriber.next(e);
});

// Any native abort signal would still work:
const controller = new AbortController();
source$.subscribe(console.log, controller.signal);

Pros: May prove to be a useful type in general outside of RxJS. APIs might be more discoverable, and it might be easier for people to move to this from Subscription.

Cons: Will require polyfills in some environments. Subclassing a type someone else owns is always a deal with the devil. We'd also be forced to subclass sometimes incomplete polyfills for some amount of time, putting us on the hook for supporting strange quirks for that.

2. Provide "helpers" for dealing with AbortSignal

Here we would provide a bunch of methods that users could use to do things like create child signals and controllers, etc.

const source$ = new Observable((subscriber, signal: AbortSignal) => {
   const resource = new Resource();
   // custom method used here.
   addToSignal(signal, () => resource.destroy());
   resource.ondata = e => subscriber.next(e);
});

Otherwise, it would be largely the same as the above.

Pros: We don't have to subclass anything. Functions aren't too hard to reason about and it would always be tree-shakable, so you only pay for what you use. We would only be relying on surface-level behaviors of any AbortSignal or AbortController instance, so less could go wrong, IMO than with subclassing.

Cons: Will require polyfills in some environments. A bunch more functions everyone needs to memorize. The suite of functions would need to have more thought put into the design (IMO) than the subclassing idea (as there we'd probably stick with known behaviors and lessons learned from Subscription).

3. Use Observable | AbortSignal to signal teardown instead

With this, we could set everything up to run off of Observable as the teardown mechanism internally. AbortSignal we could make work OOTB, by insuring that any AbortSignal passed to subscribe was automatically converted to an Observable in a seamless way. Thus AbortSignal would "just work", but so would a LOT more things. Internally, we'd need to make sure the observable was multicast/hot as we'd only want to subscribe to it once per subscription to the source.

const source$ = new Observable((subscriber, abort: AbortObservable) => {
  console.log(abort.aborted); // a boolean just like abort signal would have
  const resource = new Resource();
  abort.subscribe(() => resource.destroy()); 
  resource.ondata = e => subscriber.next(e);
});

Setting up unsubscription becomes straight forward:

source$.subscribe(observer, anyObservableInputHere);

We could also automatically convert AbortSignal internally:

const controller = new AbortController();
const { signal } = controller;
source.subscribe(observer, signal);

In theory, AbortObservable could implement everything required to make it "AbortSignalLike" for fetch use (This isn't a thing, see comments below):

const source$ = new Observable((subscriber, abort$) => {
  fetch(url, { signal: abort$ }).then(res => res.json()).then(data => subscriber.next(data))
});

Pros: No polyfills required. Easily the most flexible design. On some level, we wouldn't need to document much more than "you can pass an observable or AbortSignal in here".

Cons: Increased flexibility comes with more footguns and edge cases we'd have to account for. Observable lack an analoque for aborted that can be checked to see if a signal has been given... (this could be handled internally, however). I have a feeling that people will freakout about subscribing to an "abort observable" over muscle memory of "I must always unsubscribe from my observable subscriptions" and feel themselves going down a rabbit hole of providing abort observables to abort observables. We'll need to re-educate people around that probably.

Other thoughts:

  • We could still provide an AbortSignal or something AbortSignalLike to the Observable constructor initialization function, I suppose. That might prove useful for other APIs beyond ours (such as fetch, et al).
  • Arguably in any of the scenarios, someone could convert any Observable to an AbortSignal, and we could provide an adapter for that... But that's not that important, just food for thought.

Additional Considerations

People will probably start to want to use async/await on contructor initializers in order to use it with Promise-heavy APIs like fetch and more. This could technically work, as we don't have to return a teardown synchronously anymore, however we'd need to account for it internally (by checking for Promise returns and ignoring them).

const source2$ = new Observable(async (subscriber, signal: RxAbortSignal) => {
   fetch(url, { signal }).then(res => subscriber.next(await res.json()));
});

Related to #3122 #5545 #5683 #5591 ..

@benlesh
Copy link
Member Author

benlesh commented Oct 28, 2020

I think we should decide on the best approach above and move forward during 7.x (so long as we don't need to introduce any breaking changes).

@benlesh benlesh added AGENDA ITEM Flagged for discussion at core team meetings type: discussion labels Oct 28, 2020
@benlesh
Copy link
Member Author

benlesh commented Oct 28, 2020

FWIW: I'm most in favor of option 3 above. As I feel like it's the most flexible and it doesn't require a polyfill. However, that doesn't mean I'm not apprehensive about the possible confusion it could cause our users.

@benlesh
Copy link
Member Author

benlesh commented Oct 28, 2020

So it turns out that AbortSignalLike isn't a thing. From what I can tell, there's no way to fake out fetch into taking something that duck types as an AbortSignal. Which is really counter to almost everything else, but whatever, that's how it is. If you pass in anything but an AbortSignal created with the native AbortSignal, you will get an error: Failed to execute 'fetch' on 'Window': member signal is not of type AbortSignal.. I tried a wide variety of things, even things I'd never do, just to see if I could effect the outcome, no dice.

Likewise, subclassing AbortSignal is also not an option. If you try to subclass AbortSignal, you'll get a TypeError: Illegal constructor. So... that leaves us with options 2 or 3.

@cartant
Copy link
Collaborator

cartant commented Oct 29, 2020

I tried a wide variety of things, even things I'd never do ...

Did you try a Proxy with a getPrototypeOf handler? Just curious, really. I'm not suggesting that's something we should do.

@benlesh
Copy link
Member Author

benlesh commented Oct 29, 2020

Not a Proxy, but I did to a getPrototypeOf. What I had passed instanceof and duck-type tests. Didn't matter.

@voliva
Copy link
Contributor

voliva commented Oct 29, 2020

I'd like to point out another gotcha regarding AbortSignals with subscriptions, if I'm not wrong: You can't rely on an AbortSignal for cleaning up your resources, as the observable can also close by completing (or emitting an error) without the AbortSignal ever triggering.

The issue lies in that what Observable receives is an AbortSignal from an AbortController. AbortSignal has a property aborted, but it's read-only. The only way of making that AbortSignal trigger is through the AbortController that created it. An example:

const observable = new Observable((subscriber, signal) => {
  const resource = new Resource();
  addToSignal(signal, () => resource.destroy());

  resource.onData = data => {
    if(isError(data)) {
      subscriber.error(data);
      // Here it would also need resource.destroy()
    } else {
      subscriber.next(data);
    }
  }
})
const controller = new AbortController;
observable.subscribe(handle, controller.signal);

When resource emits something that's identified as an error, the stream would be closed through subscriber.error, but the one who owns the original AbortController is outside of that observable.

This is specially relevant when you need to close some inner subscription based on a completion of another stream:

const observable = new Observable((subscriber, signal) => {
  sourceThatCompletes.subscribe({
    next: ...,
    complete: () => subscriber.complete
  }, signal)

  sourceThatNeverCompletes.subscribe({ next: ... }, signal); // This won't be cleaned until the outer AbortController calls .abort()
})
const controller = new AbortController;
observable.subscribe(handle, controller.signal);

With a Subscription-based API these cases are covered because when you subscribe, you own that subscription, but with AbortSignal that's not the case. In order to fix this, you probably need to create an AbortController inside the observable per each new subscription you make (IMO defeating the initial purpose of using AbortSignals).


I like the third solution, as it adds more flexibility. On this line, I have some suggestions (or rather they are questions whether that's possible, as I'm probably overlooking things):

Keep teardown function (or similar) in Observable.create

Makes clean up easier in some cases

Have .subscribe also return a Subscription (or similar)

This way the consumer can close the stream in the case it doesn't have access to signal's Controller

Have the signal embedded within both Observer and Subscriber

I think this is minor, but I think that semantically it makes sense for both cases:

const observer = {
  next: () => ...,
  error: () => ...,
  complete: () => ...,
  signal // This notifies when this specific observer becomes closed
}
new Observable((subscriber) => {
  subscriber.signal // Likewise, this subscriber notifies when it becomes closed
}).subscribe(observer)

With these three changes we'd have an API very similar to the one we currently have, which would keep the existing ergonomics, but in turn it would solve the "firehose issues" easily by leveraging AbortSignal.

@benlesh
Copy link
Member Author

benlesh commented Nov 2, 2020

@voliva Yeah, it's about how you use the paradigm internally. In Rx, we'd have to create our own AbortController in each subscription that is "controlled" by the incoming AbortSignal if one is supplied. That internal AbortController would give us the signal where teardown is registered, and itself would be signaled if the user "unsubscribed" or if the source completes or errors.

(TL;DR: no concern there)

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants