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

Aborting fetch #523

Merged
merged 6 commits into from
Sep 20, 2017
Merged

Aborting fetch #523

merged 6 commits into from
Sep 20, 2017

Conversation

jakearchibald
Copy link
Collaborator

@jakearchibald jakearchibald commented Apr 9, 2017

One day this will be complete and I will retire.


Preview | Diff

fetch.bs Outdated
@@ -4683,6 +4705,7 @@ dictionary RequestInit {
DOMString integrity;
boolean keepalive;
any window; // can only be set to null
CancelationSignal signal;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is CancelationSignal rather than FetchSignal, as it'd be nice to allow the generic signal here. I'm guessing I'll be able to do something like "If signal is a FetchSignal…" later to handle things like priority changes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that would work.

fetch.bs Outdated
@@ -5343,6 +5377,13 @@ method, must run these steps:
{{Headers}} object whose <a for=Headers>guard</a> is
"<code>immutable</code>".

<li><p>Add the following steps to <var>requestObject</var>'s <a for=Request>signal</a>'s
<a>cancelation callbacks</a>:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No idea if this is an acceptable way to add a sub-algorithm to a list of algorithms.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like that is how I'd expect it to work. "cancelation callbacks" should probably have for=signal or some such, but I suspect we'll end up renaming these terms a bit before it's all done.

fetch.bs Outdated
<a>cancelation callbacks</a>:

<ol>
<li><a lt=terminated for=fetch>Terminate the request</a>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems really hand-wavey (what is "the request"?) but it's how XHR does it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I was hoping we could fix that a bit as part of doing this, but I don't have great suggestions as to how.

fetch.bs Outdated
[SameObject] readonly attribute FetchSignal signal;
// Nothing additional exposed here yet
};
</pre>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest we remove these two interfaces if we're not actually going to use them. We can add them when we add functionality that requires them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signal that appears on request objects will at some point be a FetchSignal. Is it fine for it to be a superclass of that initially?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, if you pass in the superclass or a differently subclass, presumably we'd reflect that instance directly on the constructed Request object and not some copy. And therefore that would have to specify the superclass.

fetch.bs Outdated
@@ -4705,6 +4728,9 @@ omitted from <a enum><code>RequestMode</code></a> as it cannot be used nor obser
is itself associated with <a for=Request>request</a>'s
<a for=request>header list</a>.

<p>A {{Request}} object has an associated <dfn for=Request>signal</dfn> (a {{CancelationSignal}}
object), which is initially null.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to say "null or a X object" if you make it null.

fetch.bs Outdated
@@ -4924,10 +4954,14 @@ constructor must run these steps:
to <var>method</var>.
</ol>

<li><p>If <var>init</var>'s <code>signal</code> member is present, let <var>signal</var> be it.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cannot use let twice for the same variable.

fetch.bs Outdated
<li><p>Let <var>r</var> be a new {{Request}} object associated with <var>request</var> and
a new associated {{Headers}} object whose <a for=Headers>guard</a>
is "<code>request</code>".

<li><p>Set <var>r</var>'s <a for=Request>signal</a> to <var>signal</var>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading your additions it seems you could maybe set r's signal directly each time? Or is r not set up yet?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the signal is taken from a passed-in request object r isn't set up yet.

constructor with <var>input</var> and <var>init</var> as arguments. If this throws an exception,
reject <var>p</var> with it and return <var>p</var>

<li><p>Let <var>request</var> be <var>requestObject</var>'s <a for=Request>request</a>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is just editorial, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It introduces requestObject as a variable, so I can get its signal in the proceeding steps.

fetch.bs Outdated
<li>
<p>Run the following <a>in parallel</a>:

<p><a for=/>Fetch</a> <var>request</var>.
<p><a for=/>Fetch</a> <var>request</var> and let <var>ongoingFetch</var> be the ongoing fetch.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not happy with this. The termination of ongoingFetch happens on the main thread, but I'm not sure how to get from this "in parallel" to the "abort algorithm".

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(the steps which call the abort algorithms are main thread)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you set a flag that is shared between main thread and the in parallel steps? Then you can have various "cancellation points" where you check that flag. Somewhat similar to how the implementation is done in places. Of course, cancelling the network stack doesn't work that way, but maybe you could poll the flag there.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sort-of seems to be how it's spec'd already, but using language "if fetch is terminated". I agree a flag would be better.

The bit I'm struggling with is that the signal abort steps are main thread, but the ongoingFetch is created in the "in parallel" steps.

Maybe the answer is to find a way to initiate the ongoing fetch on the main thread (which would have the terminated flag), then start the actual fetching in the "in parallel" bit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making the flag part of the underlying request that is passed to fetch? It basically acts like a signal of sorts.

See also #536 about restructuring how the fetch algorithm is invoked somewhat.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other things I wonder about:

  • How does this relate to any stream activity and how does that fall out of "terminate a fetch".
  • Do we need to change service workers at the same time to make sure this is going to work?

fetch.bs Outdated
@@ -4666,6 +4665,7 @@ interface Request {
readonly attribute RequestRedirect redirect;
readonly attribute DOMString integrity;
readonly attribute boolean keepalive;
readonly attribute AbortSignal? signal;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You haven't defined this attribute in prose.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Fixed in 5d40518.

fetch.bs Outdated
@@ -4683,6 +4683,7 @@ dictionary RequestInit {
DOMString integrity;
boolean keepalive;
any window; // can only be set to null
AbortSignal signal;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move this one up. So it stays aligned with keepalive.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what you mean by this. Do you mean I should move it just after the boolean keepalive; line? How does this make it aligned?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes it match the order for interface Request

@jakearchibald jakearchibald force-pushed the cancelation branch 2 times, most recently from 94f3f1a to 4615c57 Compare June 8, 2017 12:55
@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jun 8, 2017

I think we should spec this by giving requests their own internal abort signal. If a signal object is provided to the request, the internal abort signal will listen to this and reflect changes. This internal signal will be the one that's posted to the service worker.

Aside from chaining to the two signals together, I don't think we should use "Add the following abort steps to the signal", and instead check the signal's flag step by step:

  1. Do a small chunk of work.
  2. If signal's abort flag is set, abort somehow.
  3. Do another small chunk of work.
  4. If signal's abort flag is set, abort somehow.
  5. Another chunk of work.
  6. If signal's abort flag is set, abort somehow.

Etc etc.

This means we can be super explicit about how abort is handled & when. However, this could lead to an overly verbose spec, so I wonder if we could do something like:

  1. Let abortCheckpoint be the following steps:
    1. If signal's abort flag is not set, abort these substeps.
    2. Reject p with an "{{AbortError}}" {{DOMException}} and abort these steps.
  2. Run abortCheckpoint then do a small chunk of work.
  3. Run abortCheckpoint then do another small chunk of work.
  4. Run abortCheckpoint then do yet another small chunk of work.

Or even:

  1. Let abortCheckpoint be the following steps:
    1. If signal's abort flag is not set, abort these substeps.
    2. Reject p with an "{{AbortError}}" {{DOMException}} and abort these steps.
  2. Run abortCheckpoint before each of the following steps:
    1. Do a small chunk of work.
    2. Do another small chunk of work.
    3. Do yet another small chunk of work.

@jakearchibald
Copy link
Collaborator Author

@domenic I've experimented with granular aborting over at cancelation...cancelation-idea, specifically for the "transmit body" steps. Does this seem like a completely broken way of speccing it?

@domenic
Copy link
Member

domenic commented Jun 8, 2017

Hmm, that doesn't seem to work so great. I'd expect reading a chunk or transmitting bs to be interruptible, whereas that doesn't seem possible with that spec.

Maybe we could start by listing all the places you anticipate being able to abort? Off the top of my head:

  • Before taking any action
  • During "transmit body"
    • During reading a chunk
    • During transmitting a chunk
    • Before reading the next chunk
  • During service worker handling
  • During foreign fetch handling
  • During HTTP-network fetch's
    • During "making a HTTP request" (i.e. the step that gets the headers)
    • During the body byte receive loop
  • All of the above, but during a CORS preflight fetch

It looks to me like several of these already have provisions for the fetch being terminated during them. IMO we should just make sure to handle the rest, and then make the abort steps terminate the fetch.

@jakearchibald
Copy link
Collaborator Author

Hmm, that doesn't seem to work so great. I'd expect reading a chunk or transmitting bs to be interruptible, whereas that doesn't seem possible with that spec.

I'm trying to avoid hand-waving cancelation. Maybe we could use language like:

  1. Do some work that we can't really abort during.
  2. Let abortSteps be the following sub steps:
    1. Cancel the stream etc etc
    2. Abort these steps (would this cancel the whole algorithm?)
  3. If signal's abort flag is set, run abortSteps.
  4. Add abortSteps to signal's abort steps.
  5. Do some work
  6. Do some work
  7. Do some work
  8. Remove abortSteps from signal's abort steps.
  9. Do some work that we can't really abort during.

Maybe we could start by listing all the places you anticipate being able to abort?

That's fair. I'll change my branch to add Issue: statements at meaningful abort points.

@jakearchibald
Copy link
Collaborator Author

Done. Hopefully this will be useful.

@domenic
Copy link
Member

domenic commented Jun 9, 2017

My point is I don't think it's hand-waving. In practice all this code is running in parallel and can be interrupted by other code. That's in fact made very explicit by saying it's "in parallel". I expect it'd be implemented exactly that way.

@jakearchibald
Copy link
Collaborator Author

But surely the process for aborting is different if it's setting up a connection, vs sending a body, vs reading a body. We can't just wrap that up in a single "terminate the fetch" can we?

@domenic
Copy link
Member

domenic commented Jun 9, 2017

That's fair, we should define "terminate the fetch" to do different things depending on which phase you're in.

@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jun 9, 2017

I considered that, then thought it's easier to read if the abort steps are as close as possible to the thing being aborted.

As in:

  1. Let abortSteps be the following sub steps and add them to signal:
    1. …how to abort during task A…
  2. Do task A.
  3. Remove abortSteps from signal.
  4. Set abortSteps to the following sub steps and add them to signal:
    1. …how to abort during task B…
  5. Do task B.
  6. Remove abortSteps from signal.

Rather than:

To abort a fetch:

  1. If currently performing task A:
    1. …how to abort during task A…
  2. Else if currently performing task B:
    1. …how to abort during task B…

…then pages of scrolling away…

  1. Do task A.
  2. Do task B.

(I'm probably wrong here, just trying to figure out why)

@domenic
Copy link
Member

domenic commented Jun 9, 2017

Well, the advantage of the latter is that it seems closer to how I'd guess it will be implemented, and it creates a reusable concept of "terminate the fetch" which any spec can call (e.g. HTML's navigation algorithm can terminate all ongoing non-keepalive fetches, no matter what stage they're in).

@jakearchibald
Copy link
Collaborator Author

In terms of reusability, shouldn't it use signals? Fair enough on the implementation front, I didn't know that's how it worked.

@domenic
Copy link
Member

domenic commented Jun 9, 2017

I don't think we want HTML's navigation algorithm to depend on JavaScript-exposed objects like signals, no. Signals are just how JavaScript signals that it wants to terminate the fetch; actual fetch termination is the primitive operation.

@annevk
Copy link
Member

annevk commented Jun 28, 2017

As far as I know in implementations the caller can set some kind of flag that is shared across threads and is checked by the fetch algorithm at times where it can be terminated. Which roughly matches Jake's "internal signal" idea I think. @wanderview can correct me if wrong. I think that's how we want to define this, even if it's more verbose.

@wanderview
Copy link
Member

From an implementation perspective a simple "check a cancel flag" is not quite adequate. There are places where you can be waiting for external input (connecting a socket, etc) and not performing any kind of active local code. In these cases we would pro-actively take action to cancel like closing the socket instead of checking a flag.

I'm not sure if we really need to capture that kind of thing in the spec or not.

Talking with @annevk it sounds like we could probably immediately mark the DOM side of the fetch operation dead immediately. There is probably no need to wait for the cancel operation to completely close the socket. (Or is there?)

@jakearchibald
Copy link
Collaborator Author

I tried to capture that in #523 (comment).

The abort steps are different at different parts of a fetch. It will check a flag before it starts those steps, but it will also provide steps to run if cancellation happens during those steps. Once those steps are complete, it removes the abort steps since they no longer make sense for this phase of the fetch.

But I totally defer to whatever makes most sense for implementers. It's not something I have knowledge of. My thinking is very JavaScripty 😄 .

@jakearchibald
Copy link
Collaborator Author

I've removed termination reasons and replaced it with an "aborted flag". The next step is to PR XHR so it terminates fetch correctly and reads the aborted flag when it decides whether to fire the abort event or not.

I also noticed that the trailer promise never resolves if the response fails (without aborting). I may as well fix that while I'm here.

@jakearchibald
Copy link
Collaborator Author

XHR PR is at whatwg/xhr#152. I've also made sure the trailer promise settles.

annevk pushed a commit to whatwg/xhr that referenced this pull request Sep 8, 2017
@jakearchibald
Copy link
Collaborator Author

Btw, I don't have anything else to add to this afaik.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@domenic did you want to do another pass?

fetch.bs Outdated
@@ -1311,6 +1327,10 @@ navigate algorithm. It ensures `<code>Location</code>` has
<hr>

<p>A <a for=/>response</a> whose
<a for=response>type</a> is "<code>error</code>" and <a for=response>aborted flag</a> is set is
known as a <dfn export id=concept-aborted-network-error>aborted network error</dfn>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an*

fetch.bs Outdated
<p>If the ongoing fetch is <a for=fetch>terminated</a>, then:

<ol>
<li><p>If <var>connection</var> is not null, close <var>connection</var>.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then close*

@@ -4695,6 +4861,7 @@ dictionary RequestInit {
RequestRedirect redirect;
DOMString integrity;
boolean keepalive;
AbortSignal? signal;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this nullable and the readonly attribute is not? What does null signify vs undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const request = new Request(otherRequest);

If the signal passed to otherRequest aborts, so will request.

const request = new Request(otherRequest, {signal: null});

If the signal passed to otherRequest aborts, request will not abort.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And for the second scenario request.signal will still return an AbortSignal? That makes sense then, thanks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Sep 20, 2017

Commit message:

Abortable fetch – fixes #447.
Integrating AbortSignal.
Specifying abort points and how to handle termination.
Replacing termination reason with aborted flag.
Tests: web-platform-tests/wpt#6484.
Service worker integration follow-up: w3c/ServiceWorker#1178.

@annevk
Copy link
Member

annevk commented Sep 20, 2017

We need a little more, such as that this fixes #447.

Tests are at web-platform-tests/wpt#6484.

Follow-up is w3c/ServiceWorker#1178?

What's the status of #563? Is that follow-up?

@jakearchibald
Copy link
Collaborator Author

Updated the message. Will update #563.

@annevk annevk merged commit 0bcd5df into master Sep 20, 2017
@annevk
Copy link
Member

annevk commented Sep 20, 2017

Alright, thanks @jakearchibald!

@annevk annevk deleted the cancelation branch September 20, 2017 13:00
@jakearchibald
Copy link
Collaborator Author

Whoop! collapses

@sideshowbarker
Copy link
Contributor

sideshowbarker commented Nov 23, 2017

For the record here, for the browser engines that don’t yet support this, the following are the related feature bugs:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants