-
Notifications
You must be signed in to change notification settings - Fork 325
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 #27
Comments
Thanks for the effort folks, I'm chiming in to follow up and with a couple of questions:
Best Regards |
This is only somewhat-related to promises being cancelable. This is about cancelling a fetch. It does matter somewhat for one of the open issues and yes, we might end up having to wait or decide shipping is more important, we'll see. And we won't have a promise-subclass and a controller. Either will do. The subclass uses |
The controller approach is certainly the quickest way we'll solve this, but it's pretty ugly, I'd like to treat it as a last resort & try for the cancellable promises approach. Cancellation based on ref-countingI'm still a fan of the ref counting approach, and from the thread on es-discuss it seems that libraries take a similar approach. var rootFetchP = fetch(url).then(r => r.json());
var childFetchP1 = rootFetchP.then(data => fetch(data[0]));
var childFetchP2 = rootFetchP.then(data => fetch(data[1]));
var childP = Promise.resolve(rootFetchP).then(r => r.text());
childFetchP1.abort();
// …aborts fetch(data[0]), or waits until it hits that point in the chain, then aborts.
// fetch(url) continues
childFetchP2.abort();
// …aborts fetch(data[1]), or waits until it hits that point in the chain, then aborts.
// fetch(url) aborts also, if not already complete. Out of refs.
// childP hangs as a result
rootFetchP.then(data => console.log(data));
// …would hang because the fetch has aborted (unless it completed before abortion) Cancelling a promise that hadn't already settled would cancel all its child CancellablePromises. Observing cancellationIf a promise is cancelled, it needs to be observable. Yes, you don't want to do the same as "catch", but you often want to do "finally", as in stop spinners and other such UI. Say we had: var cancellablePromise = new CancellablePromise(function(resolve, reject) {
// Business as usual
}, {
onCancel() {
// Called when this promise is explicitly cancelled,
// or when all child cancellable promises are cancelled,
// or when the parent promise is cancelled.
}
});
// as a shortcut:
CancellablePromise.resolve().then(onResolve, onReject, onCancel)
// …attaches the onCancel callback to the returned promise
// maybe also:
cancellablePromise.onCancel(func);
// as a shortcut for .then(undefined, undefined, func) Usage in fetchFetch would return a CancellablePromise that would terminate the request onCancel. The stream reading methods If you're doing your own stream work, you're in charge, and should return your own CancellablePromise: var p = fetch(url).then(r => r.json());
p.abort(); // cancels either the stream, or the request, or neither (depending on which is in progress)
var p2 = fetch(url).then(response => {
return new CancellablePromise(resolve => {
drainStream(
response.body.pipeThrough(new StreamingDOMDecoder())
).then(resolve);
}, { onCancel: _ => response.body.cancel() });
}); |
Clearing up a question from IRC: var fetchPromise = fetch(url).then(response => {
// noooope:
fetchPromise.abort();
var jsonPromise = response.json().then(data => console.log(data));
}); In the above, var jsonPromise = fetch(url).then(r => r.json()); Now |
Since calling abort might not abort the fetch (request), I don't think the method should be called var request = fetch(url);
var json = request.then(r => r.json);
var text = request.then(r => r.text);
text.ignore(); //doesn't abort the fetch, only ignores the result. This would also work well with promise implementations that don't support cancellations (like the current spec), since calling //doSomething does not return a cancellablePromise, so calling abort won't abort what is
//happening inside doSomething. ignore makes it clear that only the result will be ignored,
//any data done can't be guaranteed to be aborted.
doSomething().then(url => fetch(url)).then(r => r.json).ignore() |
If you call it on the promise returned by Your example will fail because you have two consumers of the same stream, we reject in this case. It should be: var requestPromise = fetch(url);
var jsonPromise = requestPromise.then(r => r.clone().json());
var textPromise = requestPromise.then(r => r.text());
textPromise.abort(); In this case, I don't think "ignore" is a great name for something that has these kind of consequences. Maybe there's a better name than |
@jakearchibald, in your proposal, it looks like Does any of this flow through to the I'm also worried about subsequent uses of the original promise hanging instead of rejecting. |
@jyasskin the refcount would be increased by cancellable promises. So If you use
Yeah, it should |
'k. Say we have a careless or cancellation-ignorant library author who writes: function myTransform(yourPromise) {
return yourPromise
.then(value => transform(value))
.then(value => transform2(value)); If we had On the other hand, in a var cancellationSource = new CancellationTokenSource();
var result = myTransform(fetch(..., cancellationSource.token));
cancellationSource.cancel(); And the fetch would wind up rejecting despite the intermediate function's obliviousness to cancellation. The "revealing constructor pattern" is bad for cancellation tokens because it requires special infrastructure to be able to cancel two fetches from one point. On the other side, cancellation tokens require special infrastructure to be able to use one fetch for multiple different purposes. Either of Anne's solutions can, of course, be wrapped into something compatible with either |
Another alternative: let abortFetch;
let p = new Promise((resolve, reject) => { abortFetch = reject; });
fetch(url, { abort: p }).then(success, failure);
// on error
abortFetch(); That would make the activity on fetch dependent on a previous promise in a similar fashion (but more intimate) to this: promiseYieldingThing().then(result => fetch(url)).then(success, failure); I don't like the implicit nature of what @jakearchibald suggests here. |
TL;DR I would like to speak strongly in favor of the "controller" approach and strongly opposed to some notion of a cancelable promise (at least externally so). Also, I believe it's a mistake to consider the cancelation of a promise as a kind of automatic "back pressure" to signal to the promise vendor that it should stop doing what it was trying to do. There are plenty of established notions for that kind of signal, but cancelable promises is the worst of all possible options. Cancelable PromiseI would observe that it's more appropriate to recognize that promise (observation) and cancelation (control) are two separate classes of capabilities. It is a mistake to conflate those capabilities, exactly as it was (and still is) a mistake to conflate the promise with its other resolutions (resolve/reject). A couple of years ago this argument played out in promise land with the initial ideas about deferreds. Even though we didn't end up with a separate deferred object, we did end up with the control capabilities belonging only to the promise creation (constructor). If there's a new subclass (or extension of existing) where cancelation is a new kind of control capability, it should be exposed in exactly the same way as new CancelablePromise(function(resolve,reject,cancel) {
// ..
}); The notion that this cancelation capability would be exposed in a different way (like a method on the promise object itself) than resolve/reject is inconsistent/incoherent at best. Moreover, making a single promise reference capable of canceling the promise violates a very important tenet in not only software design (avoiding "action at a distance") but specifically promises (that they are externally immutable once created). If I vend a promise and hand a reference to it to 3 different parties for observation, two of them internal and one external, and that external one can unilaterally call That notion of trustability is one of the foundational principles going back 6+'ish years to when promises were first being discussed for JS. It was so important back then that I was impressed that immutable trustability was at least as important a concept as anything about temporality (async future value). In the intervening years of experimentation and standardization, that principle seems to have lost a lot of its luster. But we'd be better served to go back and revisit those initial principles rather than ignore them. ControllerIf a cancelable promise exists, but the cancelation capability is fully self-contained within the promise creation context, then the vendor of the promise is the exclusive entity that can decide if it wants to extract these capabilities and make them publicly available. This has been a suggested pattern long before cancelation was under discussion: var pResolve, pReject, p = new Promise(function(resolve,reject){
pResolve = resolve; pReject = reject;
}); In fact, as I understand it, this is one of several important reasons why the promise constructor is synchronous, so that capability extraction can be immediate (if necessary). This capability extraction pattern is entirely appropriate to extend to the notion of cancelability, where you'd just extract Now, what do you, promise vendor, do with such extracted capabilities? If you want to provide them to some consumer along with the promise itself, you package these things up together and return them as a single value, like perhaps: function vendP() {
var pResolve, pReject, pCancel, promise = new CancelablePromise(function(resolve,reject,cancel){
pResolve = resolve; pReject = reject; pCancel = cancel;
});
return { promise, pResolve, pReject, pCancel };
} Now, you can share the Of course this return object should be thought of as the controller from the OP. If we're going to conflate promise cancelation with back-pressure (I don't think we should -- see below!) to signal the fetch should abort, at least this is how we should do it. Abort != Promise Cancelation... Abort ==
|
Related: Composition Function Proposal for ES2016 Might be interested in the toy (but instructive) definition of Task and how it is composed using async/await. |
I don't want to deny anyone the ability to rant about |
function myTransform(yourPromise) {
return yourPromise
.then(value => transform(value))
.then(value => transform2(value));
}
myTransform(fetch(url)).cancel(); This would:
This works as expected right? |
@getify I appeal to you, once again, to filter out the repetition and verbosity of your posts before posting, rather than all readers having to do it per read. I ask not only for others' benefit, this will also boost the signal of the point you're trying to make.
Yes, that would be a specific and intentional difference between cancellable promises and regular ones. I understand in great detail that you don't like that, but can you (briefly and with evidence/example) show the problems this creates?
If you don't want to vend a cancellable promise don't vend a cancellable promise. If you want to retain cancellability, vend a child of the promise each time. |
@jakearchibald what is wrong with this way? var req = fetch('...');
// or req.headers.then(...)
req.response.then(function(response) {
if (response.headers.get('Content-Type') !== 'aplication/json') {
req.cancel();
}
return response.json();
});
req.addEventListener('cancel', function() {
// ...
});
// or Streams-like style
// closed/cancelled/aborted
req.closed.then(function() {
// ...
}); Here |
Before I get to the other points you've brought up (I have responses), let me focus on and clarify just this one:
Let me try to illustrate my question/concern (and perhaps misunderstanding). Assume: var parent = new Promise(function(resolve){ setTimeout(resolve,100); }),
child1 = parent.then(function(){ console.log("child1"); }),
child2 = parent.then(function(){ console.log("child2"); }); First, what happens here? parent.cancel(); Do both If merely passing Now, what happens if instead: child1.cancel(); Does that mean that |
@NekR that's already possible in Canary today thanks to the Streams API: fetch(url).then(response => {
if (response.headers.get('Content-Type') !== 'application/json') {
response.body.cancel();
}
}); We can already abort the response, it's the request we can't abort. The only case this is a problem is when the request is particularly large, say you're uploading a large file. It's trivial to do what you're suggesting whilst still returning a promise. The question is whether there's a benefit in the return of // If @@species is a regular promise:
var fetchPromise = fetch(url);
var jsonPromise = fetchPromise.then(r => r.json());
// To abort the request & response:
fetchPromise.abort();
// If @@species is abortable:
var jsonPromise = fetch(url).then(r => r.json());
// To abort the request & response:
jsonPromise.abort(); |
answering @getify from my POV: var parent = new Promise(function (res, rej, cancel) {
var cancelable = setTimeout(res,100);
cancel(function () {
clearTimeout(cancelable);
});
}),
child1 = parent.then(function(){ console.log("child1"); }),
child2 = parent.then(function(){ console.log("child2"); }); Since you expose cancel-ability, you setup what should happen when you cancel so that Now, as quantum physics tough us that the world is not just black or white, we also need a canceled state ( IMO™ ) as a way to ignore or react. Let's be more pragmatic, as Jake suggested already before ;-) |
Well, you get var parent = new CancellablePromise(resolve => setTimeout(_ => resolve("Hello"), 100));
var child1 = parent.then(value => console.log(value + " world"));
var child2 = parent.then(value => console.log(value + " everyone")); If parent is cancelled before resolving, it doesn't get to provide the value the others need to compose their log messages.
This isn't the case in my proposal, you can add a cancel observer in the same way you observe fulfill & reject. This would allow you to stop a spinner, but not display an error message, as cancellation often isn't error worthy.
"child2" does get printed. The parent has a cancellable promise child count of 1 ( |
The problem here is differing perspective and actor. I get the desire to want to cancel the What I am objecting to is the perspective that the code that creates In For example, one observer might say "I only wait a max of 3 seconds for a response, then I give up", but another observer may have a longer tolerance and want the request to keep going for awhile longer. |
@jakearchibald Oh right, refcount-starts-at-0 again. I think you're right. I think this all leads to the guideline that, if you have a |
Reasonable. Ideally it should, but as we all see it's a bit hard to decide right way for it. This is why I thought about returning non-promise from |
I can't think of a case you'd want that behaviour but it's Friday and I'm way over my thinking quota for the week. If you don't want children to be able to cancel the parent, cast to a normal promise before returning. function nonCancellableFetch(...args) {
return Promise.resolve(fetch(...args));
} |
But it can't. |
I haven't been convinced by this thread that we need to solve the general problem of transitive promise cancellation. That adds a lot of reference for a feature that will be rarely used. It's a poor analogy, but other multithreaded promise-like things don't have a generic cancellation. Read the c# documentation on the topic, for example. What is wrong with adding a simpler hook, solely for fetch use? |
Sorry, reference should have been complexity. I blame my phone keyboard. |
@stuartpb that's another option, yeah. The overall question is should the method of aborting a request also abort the response, or are they two separate things. If they're separate, there should be some way to join them. |
I don't see any reason a FetchController shouldn't control both, and, furthermore, allow introspection into the relationship between the two (ie. a method to query the state of the fetch, like XHR's readyState, check the progress of a response, stuff like that). |
@stuartpb this could help with upload progress too. Interesting. |
Really unsatisfying this whole thread. So much information and no outcome .. |
@jakearchibald What can I do if I want to help the spec move forward with the "revealing fetch controller constructor" design (as there appears to have been "no progress by January")? |
I think this is all we need (requires Chrome or Firefox Developer Edition at the moment). |
@jan-ivar can you please link to the spec of that :)? |
@benjamingr It's based on this discussion. |
Closing in favour of #447 as this thread's getting a little long, and contains a lot of discussion of cancelable promises that are unfortunately no longer relevant. |
AbortController
https://developer.mozilla.org/en-US/docs/Web/API/AbortController |
Goal
Provide developers with a method to abort something initiated with
fetch()
in a way that is not overly complicated.Previous discussion
Viable solutions
We have two contenders. Either
fetch()
returns an object that is more than a promise going forward orfetch()
is passed something, either an object or a callback that gets handed an object.A promise-subclass
In order to not clash with cancelable promises (if they ever materialize) we should pick a somewhat unique method name for abortion. I think
terminate()
would fit that bill.Note: the Twitter-sphere seemed somewhat confused about the capabilities of this method. It would most certainly terminate any ongoing stream activity as well. It's not limited to the "lifetime" of the promise.
A controller
The limited discussion on es-discuss https://esdiscuss.org/topic/cancelable-promises seemed to favor a controller. There are two flavors that keep coming back. Upfront construction:
Revealing constructor pattern:
Open issues
The text was updated successfully, but these errors were encountered: