-
Notifications
You must be signed in to change notification settings - Fork 296
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
Define a 'CancelationController' and 'CancelationSignal' interface. #434
Conversation
Fetch is sketching out a cancellation mechanism in whatwg/fetch#447, and other specifications are interested in cargo-culting their way to similar behaviors. To that end, this patch defines a simple interface that provides enough control for the simple case of cancellation, while paving the way for more complex consumers like Fetch to tack on arbitrary complexity.
I'm flailing a bit, @jakearchibald. I think this is more or less all we need for Credential Management, and seems like it paves the way for y'all to derive @annevk: I have no idea if DOM is the right place for this, but it seemed to fit without too much squinting. I'd appreciate your feedback regardless of where it ends up. FYI: @jyasskin, @kpaulh, @equalsJeffH, and @vijaybh as this is relevant to our ongoing CM API thread, as well as w3c/webauthn#380. |
Are we going to define a non-DOMException for the actual cancellation exception as well then so it could be factored out and move into the language if it ever comes that established? When I talked rather informally with @ajklein we could only think of Fetch as really needing this, but if more folks want cancellation (some elaboration would be appreciated) we should maybe loop in some other folks for review as well. Overall your PR looks fine, but I think we need some more abstraction for specification-consumers of the signal (that will then cancel/abort the actual operation). It's also not immediately obvious how this works where we expose the signal (a mirroring cross-thread instance of sorts I suppose) to the service worker (while the controller is on the main thread) which can then pass it along to new fetches (zero or more) which the controller will also end up affecting. |
Thanks, @annevk!
Sure. The Fetch proposal hand-waves an
WebAuthn wants
Please CC the world. :)
More non-normative notes about usage and suggested spec text for the examples? Or more something else?
It seems like that would be a Fetch-specific addition, wouldn't it? It's not clear to me that most APIs would need cross-process updates for a signal's state. I guess we can pull some sort of mirroring primitive up to DOM if that would be helpful, but it seems somewhat specific to Fetch's use case. |
I think in particular the signal object needs a hook ("abort steps" or some such) that get run when |
dom.bs
Outdated
@@ -1478,7 +1478,8 @@ steps: | |||
<ol> | |||
<li>Let <var>signal</var> be this object's {{PromiseController/signal}}. | |||
<li>Set <var>signal</var>'s [=PromiseSignal/aborted flag=]. | |||
<li>[=Fire an event=] named <code>abort</code> at <var>signal</var>.</li> | |||
<li>[=Fire an event=] named <code>abort</code> at <var>signal</var>. | |||
<li>Run <var>signal</var>'s [=PromiseSignal/abort steps=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect we probably want the event to dispatch after the abort steps have run as the event listeners can cause side effects the abort steps might not anticipate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inverted in 8681c81.
Streams is also interested in using this. I will try to review soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big-ticket items:
- This isn't specific to promises, so the names are probably not right
- Is this about cancelation or abortion? (Also naming)
- It's unclear how the abort steps would be used by other specs
dom.bs
Outdated
@@ -1419,6 +1419,103 @@ that gave folks all the wrong ideas. <a>Events</a> do not represent or cause act | |||
can only be used to influence an ongoing one. | |||
|
|||
|
|||
<h3 id=controlling-promises>Controlling {{Promise}}s</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably doesn't belong under the "Events" section. I'm not sure if it belongs in DOM at all, but I don't know of a better place really, so who knows... new WHATWG spec time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Is using IDL fine?
- How big can this possibly get?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDL should be fine. I can't imagine this getting much bigger; new spec was mostly in jest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is to say, if this remains fairly small long term, I don't like the idea of it being on its own. In DOM seems somewhat reasonable given that events are signals of sorts too, but it's pushing things a bit too of course.
And if this needs to be a non-IDL thing, perhaps Streams is a good place? It's also fits somewhat and Fetch has a dependency on that anyway, as will many other things.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it has a dependency on HTML via EventHandler, somewhat amusingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll defer to y'all about where you'd like this to live. I have no strong opinion, so long as it lives somewhere.
dom.bs
Outdated
@@ -1419,6 +1419,103 @@ that gave folks all the wrong ideas. <a>Events</a> do not represent or cause act | |||
can only be used to influence an ongoing one. | |||
|
|||
|
|||
<h3 id=controlling-promises>Controlling {{Promise}}s</h3> | |||
|
|||
Though {{Promise}} objects don't have any built-in cancellation mechanism, many {{Promise}}-vending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer "cancelation" with one "l" but as long as we're consistent we'll be good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google says "cancellation", probably because that's how the word is spelled. 🤓
dom.bs
Outdated
<dfn exception>AbortError</dfn> [=simple exception=]. | ||
|
||
<div class=note> | ||
For example, a hypothetical <code>DoAmazingness({ ... })</code> method could accept a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't capitalize doAmazingness
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
dom.bs
Outdated
{{PromiseSignal}} object. The API which wishes to support cancellation can accept such a | ||
{{PromiseSignal}}, and use its state to determine how (not) to proceed. | ||
|
||
Upon {{PromiseController/abort()}}, the relevant {{Promise}} will reject with an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't always the case, right? It depends on what the accepting method will do. I think this needs to be phrased as a convention or something, since it's not enforced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup. It feels a bit strange for DOM to be making this assertion about other specifications, but I agree with @annevk's suggestion that we should define the exception here.
dom.bs
Outdated
@@ -1419,6 +1419,103 @@ that gave folks all the wrong ideas. <a>Events</a> do not represent or cause act | |||
can only be used to influence an ongoing one. | |||
|
|||
|
|||
<h3 id=controlling-promises>Controlling {{Promise}}s</h3> | |||
|
|||
Though {{Promise}} objects don't have any built-in cancellation mechanism, many {{Promise}}-vending |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it cancel(l)ation or abortion? The API says abortion...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to change everything to cancel()
/cancellation. I used abort()
because that's what folks in whatwg/fetch#447 were saying, but I'm having trouble getting past "abortion"'s other common usage while typing it out.
dom.bs
Outdated
was initialized. When a {{PromiseController}} is created, the attribute must be initialized to a | ||
newly created {{PromiseSignal}} object. | ||
|
||
The <dfn method for=PromiseController><code>abort()</code></dfn>, when invoked, must run these |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing "method"
dom.bs
Outdated
a given API's [=PromiseSignal/aborted flag=] may need to be propagated to a cross-thread | ||
environment (like a Service Worker). | ||
|
||
The <dfn attribute for=PromiseSignal>aborted</dfn> attribute's getter must return true if the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs <code>
dom.bs
Outdated
|
||
Each {{PromiseSignal}} has an <dfn for=PromiseSignal>abort steps</dfn> algorithm which is | ||
executed when its [=PromiseSignal/aborted flag=] is set. Unless otherwise specified, this | ||
algorithm is a no-op. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, so how would this be used? It may need an example. Is the idea that e.g. fetch() would accept a signal
, then mutate its abort steps to abort the fetch? What if you pass the same signal in to multiple fetches? Maybe it should be a list of abort steps, so that consumers append to the list?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking that Fetch would subclass these and define custom steps there. Having the steps be part of the base class helps in that you don't have to mutate most other things. How Fetch handles multiple fetches and such could be done in such a setup with a single set of abort steps.
Now, I don't know for sure that Fetch would subclass these. @jakearchibald should probably weigh in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I see. Fetch would subclass, but streams (and possibly whatever @mikewest wants this for originally) would not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But maybe you're right that we should design this more so that it even works for all scenarios when not subclassed. I do think that whatever this get passed to needs to be able to hook into it, since we don't really have specification-only event listeners (I think that would be a bigger thing to add).
dom.bs
Outdated
{{PromiseSignal}}, and use its state to determine how (not) to proceed. | ||
|
||
Upon {{PromiseController/abort()}}, the relevant {{Promise}} will reject with an | ||
<dfn exception>AbortError</dfn> [=simple exception=]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't just define new simple exceptions, but I know you are aware, per your comments about hand-waving. Still, something to take care of before this gets merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to phrase this however you'd like. I'm not sure I'm up for submitting something to TC39, but perhaps there's a middle ground? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we have a middle ground between "use DOMException" and "write an ES-style spec". I guess the ES-style spec could be small though, basically just saying that it fulfills the NativeError Object Structure with name X.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But also I'm not sure if avoiding DOMException is worthwhile since we already have a dependency on EventTarget.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that is fair.
dom.bs
Outdated
@@ -1419,6 +1419,103 @@ that gave folks all the wrong ideas. <a>Events</a> do not represent or cause act | |||
can only be used to influence an ongoing one. | |||
|
|||
|
|||
<h3 id=controlling-promises>Controlling {{Promise}}s</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This actually isn't really specific to promises. For example it could be used to cancel a stream:
const readableStream = getStream({ signal });
// ... later ...
signal.abort(); // basically the same as readableStream.cancel()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CancellationController
? That seems reasonable as a base class for FetchController
if @jakearchibald needs such a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though, it's not clear to me why you'd use this model for streams when they have a built in cancellation mechanism. Do you think it's worth changing the streams API in line with what we have to do here for Promises?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea would be to allow people to pass in the signal to many APIs, streams being just one. So it's OK if an API has both a dedicated cancel mechanism and also accepts the platform-generic one, IMO.
Oh, I forgot, DOM has already set a precedent for one "l" in cancelation/canceled/cancelable, per EventInit's cancelable. (The spec also consistently uses one "l" in spec-exposed concepts.) |
sigh Ok. This is |
A single l in canceled is not a misspelling however. |
I took a look with regards to using this for WebAuthn, and I think it well meets our use case. I agree with domenic that the cancelation/cancel nomenclature is clearer than promise/abort - as odd as one 'l' looks to me right now :) |
dom.bs
Outdated
<pre class="idl"> | ||
[Constructor(), Exposed=(Window,Worker)] | ||
interface CancellationController { | ||
readonly attribute CancellationSignal signal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SameObject], right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 36028dd
dom.bs
Outdated
Each {{CancellationSignal}} has an <dfn for=CancellationSignal>cancelled flag</dfn> which is | ||
unset unless otherwise specified. | ||
|
||
Each {{CancellationSignal}} has an <dfn for=CancellationSignal>cancellation steps</dfn> algorithm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might need to be passed in through the creation of the CancelattionController. We'll see what works in prose once FetchController is specified using this.
@domenic I know you were looking at this for TC39. It feels like this is something that should be a ECMAScript built-in that has a consistent API regardless of host environment (e.g. no dependency on EventTarget). Also, for the purposes of comparison, this is the version I've been discussing with @bterlson for the last year. |
@rbuckton to repeat what I said in IRC, we tried to create a host-agnostic, ergonomic version in TC39 and failed, so we are no longer interested in pursuing that venue or approach. EventTarget is here to stay. Other hosts can copy, or not; we're not here to solve their problems, but instead to solve the web's. |
I thought about the "cancelation steps" bits this morning, and it's not clear to me that they make sense if we want the signal to be observable by multiple things at once. That is, we would either need the "list of cancellation steps" that @domenic mentioned earlier, or we need to expose an event that the consumer-side spec would listen for. Aesthetically, I prefer the latter, though I realize that it's not really a concept that exists at the moment. |
An ordered set seems fine. Whoever adds those steps should also make sure they're safe with respect to other steps. |
Per conversation with @annevk on IRC: I'd like to see a cross-realm way to unforgeably determine if the rejection reason is a cancellation or not - comparing to a string property, or using One example: if I pass a Promise across a realm - ie, from/to an iframe, or through the upcoming Realms API - without a cross-realm way to do a brand check, I'll have no way of knowing if I got a true cancellation, or if the untrusted realm I got the Promise from simply rejected it - or, if it was a true cancellation, but the untrusted realm tried to make it look like a regular rejection. |
I'm guessing |
@jakearchibald nope, since |
(As an aside, I don't think we should reuse "AbortError" DOMException even if we decided not to care about cross-realm branding, since that is already used by APIs and it's not clear they all use it with the semantics we are going for here.) @ljharb I would like to know why we should care about branding for this particular feature. It's not entirely clear to me why ECMAScript cares about it for arrays, but not all its other objects. (I might have known at some point, but I forgot.) Most platform objects don't have this kind of branding exposed either (although there's some browser engineers who think we should do that). |
@domenic fwiw, my perception is that one attempt at cancellation was tried and failed for reasons that are not entirely clear (at least to me). If there is good rationale for why this proposal is better than a host agnostic one, I'm glad to hear it, but I'll be very sad if the only reason is political. There is no doubt that the TC39 proposal that was ultimately rejected is way more involved than this proposal. I believe that a cancellation token approach very similar to this proposal has always been most likely to succeed in TC39 and, as far as I can tell, has not been tried. We should try it. What am I missing? Anyway, thinking out loud: I'm intrigued that this proposal is very similar to cancellation tokens as implemented in other systems (and as discussed by Ron and others within TC39)... but not exactly. Eg. it doesn't seem to have much support for composing cancellation signals. Has this been considered? IMO I'd like to see that the thrown thing on cancellation does not inherit from "Error". Having a separate top-level object makes checking in some cases easier, perhaps, and gives us a clean object hierarchy to extend later. It's also similar to the Ruby object hierarchy where certain classes of errors are a separate branch of an inheritance tree and are not caught by the normal "catch" block. Doing something similar might make it easier in the future to add similar language-level integration. At the very least it could make pattern matching more clear (using strawman syntax, please forgive me, and where Error here is an identifier pattern that is essentially doing an instanceof check):
|
I was supportive of @jakearchibald getting something done specifically for fetch (since that's an established API which has been waiting on cancellation for years). But if we're going to go down the path of generalizing cancellation, I agree with @bterlson that a minimal, token-based approach in ECMAScript seems like the better route here for the long-term health of the ecosystem. What I most want to avoid is producing two general but incompatible cancellation primitives, one used by web APIs and one used in ES. |
@annevk @jakearchibald
Here's a matrix of these options and constraints:
|
For what it's worth, using words with multiple commonly accepted spellings in APIs is very error-prone. Note how the title of this PR uses "Cancellation" but the PR itself mostly uses "cancelation"... If you are going to pick this name, as far as I can tell "cancellation" is far more popular (e.g. has 10x the number of Google search result hits and is the primary spelling in the dictionaries I have to hand right now), so probably you want the "ll" form. But again, I think this name is best avoided. |
TC39: tokens is actually incompatible with what we want for Fetch. We need objects that we can extend. I'm happy with TC39 uplifting what we come up with here (and leaving events and maybe the exception host-defined), but I don't want to yet again wait for TC39. I'm also happy to take any feedback into account that makes such uplifting easier. Naming: I'm happy to rename this AbortController / AbortSignal, especially if we can reuse "AbortError" as @domenic pointed out in the IDL PR. Any reason we didn't go with those names? Branding: I think this is something TC39 should solve for all objects. Once they do, we'll have it here too. See also whatwg/webidl#129. Composing cancellation signals: @bterlson can you provide an example? Error versus a new type: it seems if you just reversed the two classes in @bterlson's example and check for abort/cancel first you'd be fine, regardless of base class. (Note that I haven't checked the above with @jakearchibald and @mikewest, who are mostly driving the work here.) |
@annevk TC39 has solved it for all objects (except Error) - all objects except Error have a method somewhere that does a brand check (and either throws or returns a boolean). That's what I'm requesting HTML do too. |
@ljharb what's the cross-realm instanceof for a Map object? Is there a pattern we can follow for platform objects? |
@ljharb in that case it seems good if TC39 solved it for its all objects, including error, in that way (i.e., like |
@annevk as i said, it is solved, just not in a generic way - and the committee isn't interested (sadly) in solving it in a generic way. The method already exists - hosts can check for the presence of internal slots, and do whatever they want. They can even create their own internal slots and check for the presence of those. |
Firstly, huge thanks to @mikewest for getting this together! Naming: I'm happy with using "abort" rather than "cancel" if it gets us out of the naming issues. I personally find "cancellation" the more natural spelling, but I'd totally just deal with it (like I do with US vs UK spellings). Error type: I don't really see the issue with using Should the @slightlyoff you didn't like the naming of "signal" right? Speak now or forever hold your peace. |
@annevk: @rbuckton's https://github.com/rbuckton/prex/blob/master/docs/cancellation.md#new-cancellationtokensourcelinkedtokens has the |
@jyasskin: The semantics are a bit more complex, as it also handles unregistration of parts of the graph when the source is closed (reduces memory costs as callbacks can be garbage collected). It greatly simplifies a common pattern for moderate to advanced scenarios, but is only really useful if you have a shared and consistent cancellation API across many scenarios in an application. It is also more consistent to have the API support linked tokens if |
Note that the behavior of instanceof can be extended by hosts, or indeed by script. It's just that everyone objects to actually doing it, apparently. |
@jyasskin thanks for clarifying. That seems like something we could add later; the ability to construct a signal from multiple other signals. @rbuckton what is the reason you went with delayed notification ("promise timing")? @bzbarsky from https://twitter.com/sebmarkbage/status/851158743555358720 it seems there's at least some interest left to further explore this. The main argument I heard against is that if folks start using this it makes it harder to test things as mock objects could no longer be used. |
@annevk: originally it was not delayed, though I need to find the mail thread that convinced me to change it. A related thread about cancellation can be found here. |
Actually, that is the thread. I specifically switch to delayed notification as a result of that discussion. |
@rbuckton thanks, that's an interesting scenario. Let me try to sketch it out in terms of the API we have: const controller = new AbortController,
signal = controller.signal
signal.onabort = abortion
fetch("https://test:test/", { signal }).then(..., rejection)
controller.abort() The Over in whatwg/fetch#447 folks wanted to be able to synchronously observe when |
The above solution doesn't work of course once you pass the signal to multiple fetches (well, you'd get some kind of Promise.race thingy I suppose, whoever flips the state first wins). |
{{CancelationController/cancel()}} by rejecting any unsettled {{Promise}} with a new | ||
{{DOMException}} with [=error name=] "{{CancelationError}}". | ||
|
||
<div class=note> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<div class=example id=...>
not <div class=note>
I chatted with @domenic on IRC about the event and it was clarified to me that the primary purpose of the signal event is as a request to JavaScript-implemented signal consumers to please cancel yourself. The event is not necessarily useful for those using the We could clarify the purpose of the event by renaming it to "abortrequest" or some such. |
I'm closing this per the above comment. |
Fetch is sketching out a cancellation mechanism in whatwg/fetch#447, and
other specifications are interested in cargo-culting their way to similar
behaviors.
To that end, this patch defines a simple interface that provides enough
control for the simple case of cancellation, while paving the way for
more complex consumers like Fetch to tack on arbitrary complexity.
Preview | Diff