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

Best way to extend AbortController and AbortSignal? #13

Closed
shaseley opened this issue Mar 27, 2020 · 6 comments
Closed

Best way to extend AbortController and AbortSignal? #13

shaseley opened this issue Mar 27, 2020 · 6 comments
Labels
postTask API Related to the postTask API TaskController Related to the Task[Controller|Signal] APIs

Comments

@shaseley
Copy link
Collaborator

shaseley commented Mar 27, 2020

For the postTask API, we need to build on top of AbortSignal and AbortController to add reprioritizaton, and I'm wondering if folks have opinions on what they think the best (long-term) way to do that is.

Our current implementation matches the FetchController proposal, including the comment about FetchSignal inheriting from AbortSignal. In IDL:

interface TaskController : AbortController {
    constructor(optional TaskPriority priority = "user-visible");
    void setPriority(TaskPriority priority);
};

interface TaskSignal : AbortSignal {
  readonly attribute TaskPriority priority;
  attribute EventHandler onprioritychange;
};

But IMO the inheriting-from-AbortSignal approach comes with a few downsides:

  1. Conceptual correctness: Shouldn't a signal represent an individual event, or at least, shouldn't there be an individual signal for each event?
  2. Not having a separable PrioritySignal makes propagating individual signal components more difficult and less straightforward (see these examples)
  3. It's not clear how scalable a linear class hierarchy of inherited signals will be in the long term

An alternative is to create separate signal components (in this case PrioritySignal, or maybe TaskPrioritySignal) and optionally combine them in some way, for example a map of signals or a composite "meta-signal" using mixins.

I think ideally we'd still have a composite TaskSignal for encapsulation, but make it extend all of the signals it supports. But since multiple inheritance isn't supported, the best we could probably do is use mixins. This has the downside, however, that sharing signals is more complicated — either APIs need to be adjusted to accept the new composite signal, or the individual signal needs to be "plucked out" (e.g. AbortSignal) at callsites.

I'm starting to lean towards the composite/mixin approach, but would like it better if there were an easier way to still share signals. One option might be to create a composite parent class which exposes a .signals property, which APIs could accept and pluck signals out of (e.g. composite.signals['abort']).

I wrote up a bunch more thoughts, along with what different API shapes look like and what I think the pros and cons are.

Thoughts? Strong opinions either way?

/cc @domenic and @jakearchibald who were at one time involved in AbortSignal and FetchSignal and might have thoughts or know others that might.

/cc @jyasskin @natechapin @kjd564 who have been involved in conversations around this.

@shaseley shaseley added the postTask API Related to the postTask API label Mar 27, 2020
@domenic
Copy link
Collaborator

domenic commented Mar 30, 2020

Fascinating stuff. I'm having a hard time understanding some of what's being proposed here, as I'm not as deep into the problem and solution space as you all are. So please excuse any misunderstandings or dumb questions, and don't take my advice too seriously.

Conceptual correctness: Shouldn't a signal represent an individual event, or at least, shouldn't there be an individual signal for each event?

I don't think so, unless I'm misunderstanding. The point of AbortSignal/AbortController, at least, is allowing you to share the same AbortSignal among multiple fetches, or in general multiple abortable operations. It's a channel through which something is communicated, not a representation of any given event.

Not having a separable PrioritySignal makes propagating individual signal components more difficult and less straightforward (see these examples)

These examples are compelling, but I would solve them using "downcasting" or "separation" utility functions. E.g.:

const newAbortSignal = AbortSignal.follow(taskSignal); // only follows the abort aspect
const newTaskSignal = TaskSignal.follow(taskSignal, { ignoreAbort: true }); // only follows the priority aspect

It's not clear how scalable a linear class hierarchy of inherited signals will be in the long term

How deep or wide would we expect the hierarchy to get? I guess maybe fetch signals is our third example, hmm... but FetchSignal seems to slide in fairly well as either a subclass of AbortSignal or of TaskSignal, depending on the design.

I'm starting to lean towards the composite/mixin approach

Is this 2 or 3 in the linked doc? My gut reaction would be to prefer 3 to 2, FWIW.

@shaseley
Copy link
Collaborator Author

Fascinating stuff. I'm having a hard time understanding some of what's being proposed here, as I'm not as deep into the problem and solution space as you all are. So please excuse any misunderstandings or dumb questions, and don't take my advice too seriously.

Thanks for taking the time to read and comment, much appreciated!

Conceptual correctness: Shouldn't a signal represent an individual event, or at least, shouldn't there be an individual signal for each event?

I don't think so, unless I'm misunderstanding. The point of AbortSignal/AbortController, at least, is allowing you to share the same AbortSignal among multiple fetches, or in general multiple abortable operations. It's a channel through which something is communicated, not a representation of any given event.

Ah, I guess I think of the signals conceptually as the messages/events that are being communicated, rather than the channel itself (although this is an important concept). Maybe that understanding is based on other systems? For example, POSIX signals are "delivered". There's also the Qt framework that uses signals and slots for object communication, "A signal is emitted when a particular event occurs." That pattern also shows up in boost. With POSIX signals, I think the "channel" is process-wide, and in signals-and-slots, the slots represent the channel(s).

I wasn't sure how closely AbortSignal was modeled on existing systems, but I noticed similarities.
For example, for POSIX signals there is an abort signal (SIGABRT) for which signal handlers can be registered (analogous to 'onabort' events), and the signal is raised or "unblocked" by an abort() call (analogous to controller.abort()). This idea is then extended to other events aside from abort, but the "abort signal" represents an abort event (or a message that one occurred), IIUC.

Not having a separable PrioritySignal makes propagating individual signal components more difficult and less straightforward (see these examples)

These examples are compelling, but I would solve them using "downcasting" or "separation" utility functions. E.g.:

const newAbortSignal = AbortSignal.follow(taskSignal); // only follows the abort aspect
const newTaskSignal = TaskSignal.follow(taskSignal, { ignoreAbort: true }); // only follows the priority aspect

Yeah I think doing something like this would be preferred, and thought of something similar as I was writing up the examples. I need to think about the shape a little bit; I think the tricky case is when combining priority and abort from different sources.

It's not clear how scalable a linear class hierarchy of inherited signals will be in the long term

How deep or wide would we expect the hierarchy to get? I guess maybe fetch signals is our third example, hmm... but FetchSignal seems to slide in fairly well as either a subclass of AbortSignal or of TaskSignal, depending on the design.

That's a good question; I don't know. The FetchSignal case is interesting because it might have a different notion of priority, so I'd think it would extend AbortSignal rather than TaskSignal. But it could potentially also take a TaskSignal and use that priority, mapping it to a fetch priority. This would mean, however, that the fetch IDL would need to be updated to take any type of signal (but maybe that's okay?).

I'm starting to lean towards the composite/mixin approach

Is this 2 or 3 in the linked doc? My gut reaction would be to prefer 3 to 2, FWIW.

It's (3) in the alternative approaches section, but confusingly option4() in the examples in the appendix :(.

@annevk
Copy link

annevk commented Jun 24, 2021

Why would the IDL need to be updated? It already accepts all subclasses of AbortSignal, no?

@shaseley
Copy link
Collaborator Author

@annevk you're right, the IDL wouldn't need to be updated; the spec would need to be updated to say what to do with each type of signal.

At this point I'm happy with the inheritance approach. Being able to share TaskSignal with other APIs that accept an AbortSignal is quite nice. Still, it would be good to (eventually) consider some ergonomic additions, like the equivalent of AbortSignal.abort() for priority or a way to split off or combine the abort or priority components of a signal without using event listeners.

@annevk
Copy link

annevk commented Jun 25, 2021

See also https://github.com/whatwg/dom/labels/topic%3A%20aborting. If you want to help with those that'd be great.

@shaseley shaseley added the TaskController Related to the Task[Controller|Signal] APIs label Jun 30, 2021
@shaseley
Copy link
Collaborator Author

I'm going to close this for now as we're happy with the inheritance approach; I'll reopen if anything else comes up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
postTask API Related to the postTask API TaskController Related to the Task[Controller|Signal] APIs
Projects
None yet
Development

No branches or pull requests

3 participants