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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handing fetch termination #1178

Merged
merged 3 commits into from Nov 16, 2017

Conversation

Projects
None yet
3 participants
@jakearchibald
Copy link
Contributor

commented Aug 7, 2017

Is this a valid way of handling this, in terms of whatwg/fetch#523.

It seems too easy.

(I'll add the same to foreign fetch if this is 馃憤 )


Preview | Diff

@jakearchibald jakearchibald requested review from jungkees and annevk Aug 7, 2017

@@ -2996,6 +2997,9 @@ spec: webappsec-referrer-policy; urlPrefix: https://w3c.github.io/webappsec-refe
1. If |e|'s [=FetchEvent/respond-with error flag=] is set, set |handleFetchFailed| to true.
1. Else, set |response| to |e|'s [=FetchEvent/potential response=].
1. If |e|'s <a>canceled flag</a> is set, set |eventCanceled| to true.
1. Run the following steps [=in parallel=]:
1. Wait for the fetch to become [=fetch/terminated=].
1. [=Queue a task=] to [=AbortSignal/signal abort=] on |requestObject|'s [=Request/signal=].

This comment has been minimized.

Copy link
@annevk

annevk Aug 7, 2017

Member

It seems this algorithm has a chance to never finish.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Aug 7, 2017

Author Contributor

Yeah, I figured "GC" would take care of it.

What's the alternative? "Wait for the fetch to become [=fetch/terminated=], or (something that signal successful completion)"

This comment has been minimized.

Copy link
@annevk

annevk Aug 7, 2017

Member

I tried to think about this a bit and the way XMLHttpRequest seems to approach is that you always get a response from the fetch process, but it might be a network error annotated with a termination reason. And for the request/response bodies we got the stream that errors.

This comment has been minimized.

Copy link
@jakearchibald

jakearchibald Aug 7, 2017

Author Contributor

I could wait until on of the following:

  • Fetch is terminated.
  • "Process response" for fetch occurs, and the response type is "error".
  • "Process response done" for fetch occurs.

Then, only signal abort if fetch is terminated.

This comment has been minimized.

Copy link
@annevk

annevk Aug 7, 2017

Member

What I was saying is that currently "Fetch is terminated." is not really a signal we expose to the outside (other than the response having a termination reason or the stream erroring).

@annevk annevk referenced this pull request Sep 20, 2017

Merged

Aborting fetch #523

annevk added a commit to whatwg/fetch that referenced this pull request Sep 20, 2017

Abortable fetch
This makes the following changes:

* Integrates DOM's `AbortSignal`.
* Specifies abort points and how to handle termination.
* Replaces termination reason with an aborted flag.

Tests: web-platform-tests/wpt#6484.

Service worker integration follow-up: w3c/ServiceWorker#1178.

Fixes #447. Closes #563.

@jakearchibald jakearchibald force-pushed the fetch-abort branch from 2358afd to d4fd56b Sep 28, 2017

@jakearchibald jakearchibald changed the title WIP: Handing fetch termination Handing fetch termination Sep 28, 2017

@jakearchibald

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2017

@annevk re:

"Fetch is terminated." is not really a signal we expose to the outside (other than the response having a termination reason or the stream erroring).

I agree, but this feels like a special case since fetch called me, I didn't call fetch.

In the fetch spec I've tried to keep termination handling as close to the leaf end as I can. HTTP fetch doesn't have a "If terminates" break clause around the call to service workers, so it feels "handle fetch" should be handling the termination.

Alternatively, I could add a break clause the call to "handle fetch" in "main fetch". Then, in "handle fetch" I can somehow "handle response" for the outer fetch, and see if its abort flag is set. But this will be really messy, as the fetch spec is waiting for a response from the service worker spec, but the service worker spec is also waiting for a response from the fetch spec. I'd also end up looping the service worker response back into the service worker spec via the fetch spec, and I'd need to ignore that somehow.

I've updated the PR a little, but it seems to express the behaviour pretty clearly. It's a bit hand-wavey around getting an instance of the fetch, but I think that's part of whatwg/fetch#536 (comment).

1. Initialize |e|鈥檚 {{Event/type}} attribute to {{fetch!!event}}.
1. Initialize |e|鈥檚 {{Event/cancelable}} attribute to true.
1. Initialize |e|鈥檚 {{FetchEvent/request}} attribute to a new {{Request}} object associated with |request| and a new associated {{Headers}} object whose [=guard=] is "`immutable`".
1. Initialize |e|鈥檚 {{FetchEvent/request}} attribute to |requestObject|

This comment has been minimized.

Copy link
@annevk

annevk Sep 28, 2017

Member

Needs a trailing dot.

@annevk

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

Yeah, I guess we need to fix that issue at some point. Not looking forward to updating all the callers again.

@jakearchibald

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2017

@annevk Are you ok with:

Let |fetchInstance| be the instance of the [=/fetch=] algorithm representing the ongoing fetch.

And

If |fetchInstance| is [=fetch/terminated=], then鈥

Here?

If so I'll write some tests and get this merged.

@annevk

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

Yeah, as you said there's not really any better way to do it at the moment. Maybe note these cases in the Fetch issue so we make sure they get addressed by whatever we come up with?

@jakearchibald

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2017

@jungkees
Copy link
Collaborator

left a comment

I defer to @annevk's review for this.

@jungkees

This comment has been minimized.

Copy link
Collaborator

commented Nov 16, 2017

@jakearchibald, it seems this is ready to be merged?

@jakearchibald jakearchibald merged commit 5474d11 into master Nov 16, 2017

1 check passed

ipr PR deemed acceptable.
Details
@jakearchibald

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2017

Done. I need to take another pass at the tests. Just minor stuff though.

@annevk annevk deleted the fetch-abort branch Nov 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.