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 a fetch: The Next Generation #447

Closed
jakearchibald opened this issue Jan 4, 2017 · 240 comments
Closed

Aborting a fetch: The Next Generation #447

jakearchibald opened this issue Jan 4, 2017 · 240 comments

Comments

@jakearchibald
Copy link
Collaborator

jakearchibald commented Jan 4, 2017

We were blocked on the resolution of cancelable promises, but consensus has broken down, so we need to find a new way.

How cancellation feels

startSpinner();

fetch(url).then(r => r.json()).then(data => {
  console.log(data);
}).catch(err => {
  if (err.name == 'AbortError') return;
  showErrorMessage();
}).finally(() => {
  stopSpinner();
});

(Hopefully finally will make it through TC39).

If we want reusable code with Node.js we might want to consider actually minting a non-DOMException for this.

The fetch controller

You can get a controller object for the fetch:

fetch(url, {
  control(controller) {
    // …
  }
});

The function name above is undecided.

This controller object will have methods to influence the in-progress fetch, such as abort. These methods will influence both the request and the response stream.

Aborting an already aborted fetch will be a no-op, but there will be ways to query the final state of the fetch.

Other than the above, the design of the API is undecided.

The fetch observer

There will be an component that allows the developer to observe the progress of the fetch. This includes changes to priority, progress, final state (complete, cancelled, error etc). This component will include async getters for current state.

The observable component should use addEventListener rather than observables. We don't want a TC39 dependency, and true observables are likely to be compatible with addEventListener. We should be careful with the naming of this observer as a result.

It's undecided how you'll get this object when calling fetch. The controller may extend it, the controller may have it as a property, or the two objects may be offered independently.

Other than the above, the design of the API is undecided.

Controlling multiple fetches at once

It should be relatively easy to cancel three fetches at once, or change their priority at the same time. If this is complicated to do in a low-level way, we may consider ways of creating a single controller that can be provided to multiple fetches, or a way to allow a controller to mimic another fetch through its observer.

How this happens is undecided.

Within a service worker

The observer component will be available to a fetch event via a property.

addEventListener('fetch', event => {
  console.log(event.fetchObserver);
});

The property name above is undecided.

This should allow the service worker to hear about the page's intentions with the fetch in regards to priority and cancellation, and perform the same on the fetches it creates.

It seems unlikely that event.respondWith(fetch(event.request)) would give you any of this for free.

@jakearchibald
Copy link
Collaborator Author

Having thought about it while writing the above, my gut-preference is for method(s) on the request object.

It leaves the door open for cancellation tokens, which provide a better solution for service workers and response methods like response.json(). In the meantime, if developers want to cancel the response, they can use the already-available streaming methods rather than the helper functions like response.json().

@jakearchibald jakearchibald changed the title Aborting a fetch Aborting a fetch: The Next Generation Jan 4, 2017
@stuartpb
Copy link

stuartpb commented Jan 4, 2017

Re: service workers watching for changes to a fetch:

How do you feel about the event object itself having an addEventListener method?

@delapuente
Copy link

When talking about the SW case, do you mean aborting the request sent to the network by the fetch handler due to external reasons?

@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jan 4, 2017

@stuartpb

How do you feel about event having an addEventListener?

This is for the service worker case right? Yeah, I had the same thought. I dunno if that's possible. But if we need to go down that route it could be some kind of observable thing instead.

@benjamingr
Copy link
Member

I think Observables want them, so they may come back to life

Nope. Not going to explore that route closely. I recommend we don't base this API on CancelToken.

@jakearchibald
Copy link
Collaborator Author

@delapuente

When talking about the SW case, do you mean aborting the request sent to the network by the fetch handler due to external reasons?

Yeah. For example, I may react to a fetch for /article by fetching 3 different things, /article-start, /article-middle, /article-end. If the user hits the big "stop" button in their browser, or closes the tab, it'd be nice to know that happened and abort the related fetches.

@jakearchibald
Copy link
Collaborator Author

@benjamingr thanks, I'll update the OP.

@benjamingr
Copy link
Member

Having thought about it while writing the above, my gut-preference is for method(s) on the request object.

Personally, I'm in favor of that solution and it's also how it's typically implemented in other languages. I don't mind having programmers type a little more if they need abort functionality.

I'm also fine with the solution @jar-ivan suggested:

request(uri, {
  cancel: somePromise
})

Where somePromise fulfilling cancels the request.

@stuartpb
Copy link

stuartpb commented Jan 4, 2017

Okay, so, one thing I think is kind of muddying this design a bit, in terms of solutions that work for SW fetch events and fetch()-initiated requests:

Service workers shouldn't have control over incoming requests.

They can do whatever they want with the response the SW produces for the request, of course. If the SW wishes to "abort" an incoming request, it can respond with an appropriate 5XX(?) code denoting "Request Terminated" (and it could also pass some out-of-band signal back to whatever originated the request that would make that entity abort the request).

And of course, a SW can control requests initiated the service worker, using fetch() - that's no different from the non-SW fetch() use case.

But, as far as the incoming request goes, while there's certainly sense in making it possible to monitor the request's status, I think that listening is as far as the abstraction should sensibly go. A request coming into a SW should be effectively read-only, beyond control - the solution should be not just something that can differ from fetch(), it must differ (though the solution for fetch() may take on a superset of the incoming-request-introspection).

Although Service Workers have a privileged position in the browser's plumbing, in terms of design I still mentally model them as, effectively, a local proxy server. You can write a proxy server that stops listening to a request, but you can't make a server that controls the UA making a request.

@jan-ivar
Copy link

jan-ivar commented Jan 4, 2017

I'm also fine with the solution @jar-ivan suggested

This one for reference.

@wanderview
Copy link
Member

I still dislike mutating the Request with state like cancelation, etc, but I'm willing to accept it at this time. I just wish we had an object that represents the on-going fetch activity. Request is more "a fetch that can be performed" and a Response is "a fetch that has been performed".

If we go with Request.cancel(), please include a Request.cancelled getter determining the current state as well.

Also, should we allow script to tell if a Request is completed and cancel() will effectively be a no-op?

Service workers shouldn't have control over incoming requests.

I'm not sure I understand this assertion. The entire ServiceWorker interception system is to allow it to gain some control over how that incoming request is processed.

@benjamingr
Copy link
Member

Also, should we allow script to tell if a Request is completed and cancel() will effectively be a no-op?

I'm +1 on both.

@stuartpb
Copy link

stuartpb commented Jan 4, 2017

To TL;DR my last comment a bit, we need to recognize that there are four concerns in play here, which I believe will be best served if they're recognized orthogonally:

  • Monitoring an incoming request (in service workers)
  • Controlling an outgoing request
  • Monitoring an incoming response from an outgoing request
  • Controlling an outgoing response (from an incoming request to a service worker)

Two of these concerns are about monitoring something out of the current code's control, two of these things are about controlling something the current code initiated, two of them are about requests, and two are about responses - but these are four different cases, and an appropriate design for any of these concerns needs to recognize which elements of the case it shares with which of its siblings.

@taralx
Copy link

taralx commented Jan 4, 2017

I'm personally against modifying the Request object, both because it fails to cancel the response (which will surprise people) and because it interacts badly with the various ways in which Requests are copied/cloned.

The other solutions are all trade-offs, but so far I like either the controller pattern or the cancelling promise (@jan-ivar) pattern. The latter has the advantage of working with service workers:

addEventListener('fetch', event => {
  event.respondWith(
    fetch('cat.jpg', {cancel: event.cancelled})
  );
});

@mkruisselbrink
Copy link
Collaborator

I agree that modifying the Request object feels very wrong (why not go all the way back to the XHR api...). I kind of like the cancelling promise pattern. It's conceptually simple and easy to understand (well, not sure what I would intuitively expect when the cancel promise is rejected, but other than that...), while providing all the necessary functionality.
It would of course change the fetch API slightly (since now the second parameter can no longer be just a RequestInit dictionary, but instead would need to be some kind of FetchOptions dictionary), but that doesn't seem problematic.

@annevk
Copy link
Member

annevk commented Jan 5, 2017

I think there's a strong argument for having something that represents the ongoing fetch:

  • Cancelation should affect the overall fetch, not the request.
  • Observing cancelation state and done state through getters.
  • The additional use cases centered around fetches (push, changing priority on the fly, observing progress).

Putting that on Request would get quite ugly. (Especially considering the Cache API and short lifetime of Request objects (fetch() creates a new one from the one it is passed).)

@jakearchibald
Copy link
Collaborator Author

@benjamingr @jan-ivar I've updated the "token"s proposal in the OP to use the promise-based pattern you suggested. Cheers!

@jakearchibald
Copy link
Collaborator Author

@mkruisselbrink when you say "I kind of like the cancelling promise pattern", which pattern are you referring to?

@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jan 5, 2017

I agree with @annevk that having a representation of an ongoing fetch is the least hacky way to do this. However, I don't like the way you end up passing the controller out of the function that creates it:

// eww
let controller;
fetch(url, {
  control(c) {
    controller = c;
  }
});

This brings me back to "Add a method to the returned promise". We could make fetch() return a controller object that subclasses Promise. Its @@species would be Promise, so .then() would return a regular promise.

const controller = fetch(url);
const p = controller.then(response => );
controller.abort();
p.abort === undefined;

Something like controller.observer could contain the "read-only" parts of the controller. This is where we could provide upload progress, changes to priority etc. You could safely pass this to other pieces of code allowing them to observe the fetch without being able to modify it.

const controller = fetch(url);
// Hand waving through the API a bit…
controller.observer.complete; // a promise that resolves, or rejects with a TypeError (network error) or AbortError (cancelled)
controller.observer.uploadProgress.subscribe();
controller.observer.priorityChange.subscribe();

This could also appear on the fetchEvent, solving the service worker case:

addEventListener('fetch', event => {
  const catFetch = fetch('cat.jpg');
  event.fetchObserver.complete.catch(err => {
    if (err.name == 'AbortError') catFetch.abort();
  });

  event.respondWith(catFetch);
});

You could even have a helper method to couple a fetch observer to another fetch. So changes to one fetch happen in other(s).

We could ship the controller first, and leave the observable until later, especially if we want to use TC39 observables.

Cons:

This is a pretty big change. One of the points of contention in the previous thread was allowing someone you gave the promise to, to affect the outcome of the promise (eg, aborting it). Whoever you pass the promise to can already lock & drain the response stream, so it isn't really immutable. If you're passing a fetch promise to multiple people and expecting them to all be able to independently handle the response… you're going to have a bad time.

If this becomes a blocker, the return type could switch on fetch(url, {controllable: true}), which is a little hacky, but IMO less hacky than the callback.

@stuartpb
Copy link

stuartpb commented Jan 5, 2017

Passing the controller out of the function that creates it doesn't seem hacky at all to me - certainly no hackier than the way the new Promise constructor already works. It's certainly less hacky than changing the established return type of fetch().

@stuartpb
Copy link

stuartpb commented Jan 5, 2017

If revealing constructors bother you so much, we could do something where you can also pre-construct the controller via new FetchController, which you then pass as control (kind of like a request token).

Of course, this makes it so you run into a whole thorny woods of potential mis-uses like attempting to use the same FetchController for multiple fetches. The revealing constructor makes it so that you're guaranteed to only have a 1:1 controller:fetch relationship, much like how resolve and reject are directly tied to their new Promise.

Of course, you could want to design for a world where one controller can control multiple Fetches (it's a use case you described for tokens above), but then you'd have to grapple with all the things that breaking that symmetrical assurance would ruin, like observer.

@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jan 5, 2017

@stuartpb I rarely need to pass resolve/reject out of the promise constructor, since the promise-related work is done within the constructor function. That doesn't seem anywhere near as likely here.

we could do something where you can also pre-construct the controller

What are the pros of that vs returning a promise subclass from fetch(), as detailed above?

@stuartpb
Copy link

stuartpb commented Jan 5, 2017

I'm not certain that fetchEvent should have the same kind of observer as an outgoing fetch, since, as I noted earlier, those are fetches in opposite directions, privy to data of opposite natures (for a ServiceWorker, uploadProgress would represent data we're getting/seeing, but to a fetch caller, it represents data we're sending/producing). Maybe two sibling classes that inherit from a common parent.

@delapuente
Copy link

If this becomes a blocker, the return type could switch on fetch(url, {controllable: true}), which is a little hacky, but IMO less hacky than the callback.

Sharing my thoughts. I think cancellable promises are not the right way. I would keep Promises as values-in-time abstractions and I would not mess with fetch-specific details.

I always thought fetch API is missing an intermediate type to represent the ongoing operation so I'm more inclined to create this control object once and for all.

To be honest, I really like the new FetchControl() option proposed by @stuartpb It's optional, enables progressive adding of functionality (like observers as @jakearchibald suggested), leverage separation of concerns and encourage dependency inversion.

In the future, it could offer the possibility of controlling multiple requests (to cancel all the fetches involved in the start/middle/end example) and keeps regular fetch mechanisms lightweigh.

Following @wanderview suggestion, I would make .cancel() to return a promise which resolves if the fetch is actually cancelled or rejects with a reason (no op) if cancellation is not possible.

For the big-stop-button case. I'm not sure if we should allow the requests to be continued. I'm happy with providing a way to notificate the SW about this action (or page closing) but perhaps we should stop all ongoing fetches since this is user's will.

In this case, maybe hardstop and allclosed could be functional events.

If I haven't convinced you, I'm more inclined for @stuartpb event.addEventListener() example.

Hope it helps.

@jeffbski
Copy link

jeffbski commented Jan 5, 2017

IMO aborting a fetch should signal a cancel stopping any upload of data from the request but also preventing the download of data from the response.

For me that's the most important aspect of this, that it immediately stops both upload and download so we are not wasting bandwidth and freeing up the connection. This is super important for devices on slow networks and where bandwidth is metered by usage.

If we only wanted to ignore or error the use of the response then that can be easily done in user land by wrapping the result in an observable. Thus we can do this today without any changes. Of course it would be nice to have a built-in standardized way to do this in the API, but nevertheless it can be done today as is.

So I would hope that it remains a goal of this feature to stop the data transfers of both uploads and downloads, that is the more important goal in my opinion.

@jakearchibald
Copy link
Collaborator Author

@delapuente "cancelable promises" are not being suggested here. They've been abandoned. Or are you calling the FetchController promise subclass a "cancelable promise"?

In the subclass proposal, the returned value from fetch() becomes the intermediate type you're wanting.

const controller = fetch(url);
const p = controller.then(response => );
controller.abort();
p.abort === undefined;

@jan-ivar
Copy link

jan-ivar commented Jan 5, 2017

@jakearchibald A controller that is also a promise, is magic. I think that's begging for trouble.

I can see SO filling up with questions like:

const controller = fetch(url).then(response => );
controller.abort(); // Why is abort undefined???

Also, we should be forward-looking and use async functions in examples IMHO. Today I write:

let response = await fetch(...args);

Accessing the magic controller would force me to do this:

let controller = fetch(...args);
let response = await controller;

Which again is easy to get wrong:

let controller = await fetch(...args);
let response = await controller;
controller.abort(); // Why is abort undefined???

To await or not to await? Considering fetch is an asynchronous function, and a controller is a promise, I wouldn't blame anyone for getting it wrong.

@benjamingr
Copy link
Member

🎉

@sompylasar
Copy link

I know I'm late for the party and missed something, but why the attribute name was chosen to be just signal and not, for example, abortSignal? If at any future point there will be a need to provide other signals to fetch, how is this change planned to be made without breaking compatibility? I saw somewhere that the type AbortSignal may be changed to a FetchSignal, how would this look like? Thank you.

@stuartpb
Copy link

@sompylasar The signal object will handle the propagation of all fetch control signals, not just aborting - the idea is that you'd want to keep a set of fetches in general lock-step.

@jakearchibald
Copy link
Collaborator Author

Yep, if we introduce FetchController its .signal will be a FetchSignal which inherits from AbortSignal.

@wzup
Copy link

wzup commented Feb 23, 2018

@jakearchibald

Chrome will start soon.

Guys from Google say it already work, but it still doesn't as of Chrome 63.

@annevk
Copy link
Member

annevk commented Feb 23, 2018

@wzup the article says it works in Edge and Firefox, and points to https://bugs.chromium.org/p/chromium/issues/detail?id=750599 for Chrome, which isn't marked fixed yet...

@joeyparrish
Copy link

It looks like a change for this was landed on Feb 5, so based on the Chromium release schedule, I am guessing that it would show up in Chrome 66. Just a guess.

@Bnaya
Copy link

Bnaya commented Feb 23, 2018

it's on chrome canary (66)

@MattiasBuelens
Copy link

MattiasBuelens commented Feb 23, 2018

@joeyparrish That change added the AbortController and AbortSignal APIs, but it is not yet integrated with fetch. From the commit message:

One fetch() wpt test that previously failed due to AbortController being undefined now times out because the "signal" properties in fetch options doesn't do anything yet. This will be fixed once it is implemented.

If you want, you can already write your own functions that can be aborted through an AbortSignal. However, Chrome's fetch doesn't yet know about AbortSignals, so you can't yet abort a fetch with it. (Hopefully that changes soon?)

@joeyparrish
Copy link

Ah, okay. Now I get it. Thanks for clarifying.

@Mouvedia
Copy link

Mouvedia commented Jun 10, 2018

That change added the AbortController and AbortSignal APIs, but it is not yet integrated with fetch.

@MattiasBuelens Which means we can't use the existence of the AbortController constructor as a mean to check whether abortion is supported?

@MattiasBuelens
Copy link

@Mouvedia No, you can't. However, you can check if Request.prototype.signal exists, like in yetch. 😉

@Mouvedia
Copy link

I couldn't find a reference to abortions automatically triggered by the browser in the specification.

What happens to ongoing requests when you navigate away or close a tab?
Should the abort event (of the signal) fire?
Do I have to rely on onbeforeunload?

@annevk
Copy link
Member

annevk commented Dec 21, 2018

That's #153 and there's various issues against whatwg/html too. Someone needs to do a lot of research across browsers to get that nailed down more exactly.

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

No branches or pull requests