-
Notifications
You must be signed in to change notification settings - Fork 288
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
Abort controller #437
Abort controller #437
Conversation
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.
We should also update the Abstract to account for this new feature.
dom.bs
Outdated
@@ -660,7 +660,7 @@ flags that are all initially unset: | |||
<ul> | |||
<li><dfn export for=Event id=stop-propagation-flag>stop propagation flag</dfn> | |||
<li><dfn export for=Event id=stop-immediate-propagation-flag>stop immediate propagation flag</dfn> | |||
<li><dfn export for=Event id=canceled-flag>canceled flag</dfn> | |||
<li><dfn export for=Event id=canceled-flag>aborted flag</dfn> |
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 and similar changes to Events will need to be reverted. Simple search & replace is too simple unfortunately.
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.
Oh ffs, sorry. I should have paid more attention to the patch.
dom.bs
Outdated
|
||
Each {{AbortSignal}} has <dfn for=AbortSignal>abort callbacks</dfn>, which is an [=ordered set=] of | ||
algorithms which are to be executed when its [=AbortSignal/aborted flag=] is set. Unless otherwise | ||
specified, its value is the empty set. |
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.
Can we name this something different from callbacks. Callback already has a very specific meaning in IDL.
Shall we rename the event to "abortrequest" to more clearly indicate it's a hook that needs to be implemented and not a hook that will tell you if something actually happened? See #434 (comment) onward for rationale. |
Does "abortrequest" sound too much like "abort the network request"? What about "signalabort", along with "signalprioritychange" etc etc |
Given the choice, I'd favor sticking with "abort" and explaining what it means in a note or so (that it's a way for functions that accept signals to hook into the aborting mechanism). |
7e58b74
to
bd99c3f
Compare
dom.bs
Outdated
</dl> | ||
|
||
The <dfn attribute for=AbortController><code>signal</code></dfn> attribute must return the value to | ||
which it was initialized. When a {{AbortController}} is created, the attribute must be initialized |
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.
"an AbortController"
dom.bs
Outdated
to a newly created {{AbortSignal}} object. | ||
|
||
The <dfn method for=AbortController><code>abort()</code></dfn> method, when invoked, must <a | ||
for=AbortSignal>signal abort</a> on this object's {{AbortController/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.
You can probably write the link as [=AbortSignal/signal abort=]
, although I haven't tested that with DOM's build system.
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.
@annevk what's your take on shortcuts like this? I use them in the service worker spec and find them easier to read/write, but then I lean heavily on the markdown stuff too. Do you have a preference for this spec?
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 mostly use HTML (but also {{object}}) and I actually prefer explicit <p>
starting tags too, but I figured I could add those once we're good to go.
I wouldn't particularly mind if you used such shortcuts though. I do mind newlines inside tags or phrasing-content elements.
dom.bs
Outdated
interface AbortController { | ||
[SameObject] readonly attribute AbortSignal signal; | ||
|
||
void abort(); |
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 agree with your choice to return void
here. In particular, I expect that returning "a Promise that is settled when cancellation has completed" will encourage developers to miss the fact that "an API listening to a signal may choose to ignore it" and so write bugs.
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.
@jyasskin The result of cancel()
in prex is to observe any errors thrown in the registered callback (or here, the onabort event), and respond accordingly. In prex, registered callbacks are not evaluated immediately and as a result the only way to observe the exception is through a Promise. A Promise return wouldn't strictly be necessary if cancel()
ran to completion synchronously.
#438 is the corresponding issue. |
I addressed all nits I had I think. I also posted a design for serializing and deserializing |
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.
LGTM with nits. Let me try applying it to streams.
dom.bs
Outdated
|
||
<dt><code><var>controller</var> . </code>{{AbortController/abort()}} | ||
<dd>Invoking this method will set this object's {{AbortSignal}}'s [=AbortSignal/aborted flag=], | ||
thereby signaling to any observers that the associated activity should be aborted. |
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.
Nit: I think this would be clearer as "... set this AbortSingal's aborted flag, and signal to any observers..." (instead of using "thereby")
dom.bs
Outdated
|
||
<p>An {{AbortController}} object has an associated <dfn for=AbortController>signal</dfn> (an | ||
{{AbortSignal}} object). When an {{AbortController}} object is created, its | ||
<a for=AbortController>signal</a> must be set to a new {{AbortSignal}} object. |
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.
Do you want to define the constructor more explicitly, perhaps?
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.
Yeah, that sounds like a good idea. Currently it's not defined at all.
dom.bs
Outdated
<p>Any web platform API using promises that to represent operations that can be aborted must accept | ||
{{AbortSignal}} objects, use the [=AbortSignal/abort algorithms=] mechanism, and convey that the | ||
operation got aborted by rejecting the promise with the "{{AbortError}}" {{DOMException}}. | ||
|
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.
It might help to also suggest how the signal is accepted, so that we don't sometimes name it signal
and sometimes abortSignal
. Based on the example it sounds like signal
is the way to go.
<li><p>Set <var>signal</var>'s [=AbortSignal/aborted flag=]. | ||
|
||
<li><p><a for=set>For each</a> <var>algorithm</var> in <var>signal</var>'s | ||
[=AbortSignal/abort algorithms=]: run <var>algorithm</var>. |
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.
Probably you want to clear the abort algorithms here. It's unobservable, but seems like good practice.
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.
If we do that, we probably shouldn't allow adding new abort algorithms either if the aborted flag is set. Should we have a dedicated add algorithm folks can use?
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 yeah, good point. We could do that, or we could go with a different strategy, of noting that abort algorithms will never fire after the aborted flag is set, so even though the spec doesn't clean them up explicitly, implementations probably should. I guess that's not as good though, once I typed it out.
Addressed the remaining feedback and also created tests. It seems we should land this together with Streams (maybe Streams a day later for Shepherd) and then file browser bugs for the combined feature. So each bug would point to this change and the change in Streams, and the tests for this change and the tests for the change in Streams. Does that sound okay to everyone? |
dom.bs
Outdated
<ul class=brief> | ||
<li>Accept {{AbortSignal}} objects through a <code>signal</code> dictionary member. | ||
<li>Use the [=AbortSignal/abort algorithms=] mechanism. | ||
<li>Convey that the operation got aborted by rejecting the promise with the "{{AbortError}}" |
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.
Nit: an "AbortError" DOMException, not the "AbortError" DOMException.
dom.bs
Outdated
@@ -1534,6 +1534,16 @@ unless specified otherwise. | |||
<a for=/>set</a> of algorithms which are to be executed when its [=AbortSignal/aborted flag=] is | |||
set. Unless specified otherwise, its value is the empty set. | |||
|
|||
<p>To <dfn for=AbortSignal>add</dfn> an algorithm <var>algorithm</var> to an {{AbortSignal}} object |
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.
export
please
dom.bs
Outdated
<li><p>Run <var>algorithm</var>. | ||
|
||
<li><p>Remove <var>algorithm</var> from <var>signal</var>'s | ||
<a for=AbortSignal>abort algorithms</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.
Mutating a list as you loop through it, although it's probably well-defined, can be confusing, and easy to implement correctly. I'd suggest just clearing the list after the loop.
I think this will be more compelling if we could also land it together with aborting fetch(). However, I realize you are leaving for vacation soon so that might not work out timeline-wise. Note also that only Blink has a pipeTo implementation. (Safari has one, but it is outdated and relies on an outdated WritableStream implementation, so effectively it doesn't really work...) So only Blink would be able to implement the streams-side change. @yutakahirano @tyoshino @ricea for an idea on whether implementing this in order to cancel streams and/or fetch is a good near-term goal. Maybe we and/or @jakearchibald could help with the spec work on canceling fetch while you're unavailable. |
<p>An {{AbortSignal}} object has an associated <dfn for=AbortSignal>aborted flag</dfn>. It is unset | ||
unless specified otherwise. | ||
|
||
<p>An {{AbortSignal}} object has associated <dfn for=AbortSignal>abort algorithms</dfn>, which is 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.
Hmm. This isn't exported, so I can't remove my algorithm from it. Maybe we should define "remove an algorithm from an AbortSignal" as well. Then it can stay un-exported.
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.
If removing doesn't have any conditions I'll just export this instead.
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 don't think it does, but it's kind of nice to say that the internal list of abort algorithms is an implementation detail, and other specs only interact by adding and removing. E.g. this way other specs cannot empty the list of algorithms, or reshuffle them.
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.
In Fullscreen we want the other way. Maybe we should revisit that.
cc @foolip
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.
@annevk, you mean whatwg/fullscreen#79 + whatwg/html#2650, right?
Whether to expose the data structure directly or wrap in a set of abstract operations I think won't have the same answer every time, but I suppose the more different specs that interact with a thing the more I'd lean towards abstract operations.
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.
Yeah, I guess it's okay for it to be a little different.
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.
FWIW, I agree with abstract operations here. Prevents other specs setting "abort algorithms" etc etc.
I can look into aborting fetch() to at least get an idea of what's needed, but I thought the plan was that we'd sort through the remainder of that in July, whereas we'd want to do this quickly to avoid blocking Streams. |
Yeah, that's fair; upon re-reviewing the threads I forgot how complicated the plans were for fetch. |
I looked into whatwg/fetch#523 and while it's somewhat close if you don't stress the somewhat horrible "prose abort API" (terminate a fetch), there's a few things left to figure out, including service workers. That's not going to happen within a week. So it's either this and Streams or just let it all be for another 5-6 weeks. |
|
||
<p>An {{AbortSignal}} object has associated <dfn for=AbortSignal>abort algorithms</dfn>, which is a | ||
<a for=/>set</a> of algorithms which are to be executed when its [=AbortSignal/aborted flag=] is | ||
set. Unless specified otherwise, its value is the empty set. |
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 too familiar with the spec language here, but this sounds like it encourages other specs to replace the set, but in reality we only want other things to add/remove item.
Should this say something like "The set is initially empty"?
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.
Following the above discussion, I guess other specs can't replace it.
I've added some example spec text in 51b8ca1 which could do with a review. |
dom.bs
Outdated
<li>If |options|' <code>signal</code> member is present, then: | ||
<ol> | ||
<li>If |options|' <code>signal</code>'s [=AbortSignal/aborted flag=] is set, throw an | ||
"{{AbortError}}" {{DOMException}}. |
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 worth adding a note here to point out this will be turned into a rejection, or is that assumed knowledge?
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 think it's not as widely appreciated as it should be and could use a note. Folks tend not to know about the "If op has a return type that is a promise type" part of https://heycam.github.io/webidl/#dfn-create-operation-function.
Or you could be less subtle: wait to create p until you need to store it or go in parallel, and "return a promise rejected with ..." here.
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.
IMO relying on automatically turning things into a rejection is a spec bug that Web IDL catches for you. We should go with @jyasskin's suggestion instead.
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.
Cheers! I've added it as a 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.
@domenic ohhh really? In JS I consider:
async function blah() {
return Promise.reject(Error('broke'));
}
…an anti-pattern compared to:
async function blah() {
throw Error('broke');
}
I was applying the same logic to specs. Are specs just different, or am I wrong when it comes to JS too?
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.
Specs are different; we need to be more explicit.
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.
Fair enough. Will change.
I cleaned up your example text a little bit with a more preferred pattern (we'd rather return at the end, since that signals the end of an algorithm). It's probably still good to wait with landing this until either Streams or Fetch is ready to go (and has tentative implementer commitments). If everything is okay now I might squash the commits and update the commit message to point to the tests. (Also, if we don't do anything Mike will be the author of the final commit. That's okay with me, but I'd also be fine with changing it to Jake. The commit message itself already acknowledges the joint effort.) |
Streams has Blink commitments at least. |
@kpaulh / @equalsJeffH if this lands, would you expect to get FF/Edge/Blink commitments for it in the first version of WebAuthn, or only in the second? |
@annevk I don't mind who's name is on the commit. |
New APIs for a generic reusable abort mechanism. Both Fetch and Streams will build on this initially. Tests: web-platform-tests/wpt#5960. Fixes part of whatwg#438. (This commit is also authored by Jake and Anne.)
The commit is ready to go though further review appreciated. Firefox would likely first ship aborting |
It seems that for |
Some additional info: We expose signals on request objects so they're (eventually) visible within a service worker, and signals the page's intent to abort the request. We use a copy because we can't use a reference once the request goes to a service worker. Also, we have plans to introduce a |
From that description the pattern doesn't seem generally applicable to me. |
My expectation is that this would be part of the second version of WebAuthN. @equalsJeffH do you agree? |
@domenic or I will likely land this later today, given that writable streams has enough commitments to use it. (Cancelable So if there's any concerns left now would be the time. |
Sigh. Because I edited the commit before landing it's now attributed to Jake and not Mike. Oh well, new objects everyone! |
Branched from #434
Preview | Diff