whatwg / fetch

Fetch Standard
https://fetch.spec.whatwg.org/
Other
2.11k stars 329 forks source link

Add timeout option #20

Closed wheresrhys closed 2 years ago

wheresrhys commented 9 years ago

Most ajax libraries and xhr itself offer a convenient way to set a timeout for a request. Adding timeout to the properties accepted in the options object therefore seems like a sensible addition to fetch.

https://github.com/github/fetch/pull/68

domenic commented 9 years ago

Ideally we would have a design for cancelable promises that fetch could use. But unfortunately there are still four or five competing proposals thrown out and every time a web developer asks me "why can't we do cancelable promises?" and I look into exactly what they're asking for I end up adding a new proposal to that list. It would take some concerted effort to try to find something good.

In the meantime I think a sensible cancelation strategy is to reject the promise with some sort of error. The actual cancel() method if it exists should not be on the returned promise though, at least not until we figure out cancelable promises. That leaves a couple options I can see:

As for the error itself we could future-proof there slightly better. If we create a new Error subclass called e.g. Cancellation that follows all the normal ES conventions for Error subclasses, in the worst case it ends up being fetch-only but in the best case ES ends up using it for cancellable promises.

matthew-andrews commented 9 years ago

Don't let people cancel requests until the headers come back and you have a response object. In this case you could use the built in stream cancelation of ReadableStream: res.body.cancel().

This doesn't feel particularly useful… the cases where you would want a fetch to timeout would be when the network conditions were not good and you weren't getting anything back…

Given timeout is such a common use case, even if promises had a concept of being cancelled providing a timeout option seems immensely useful… what am I not seeing here?

kornelski commented 9 years ago

To me it seems that having API to cancel started fetch is completely separate from request timeouts. Mechanism used for cancellation on timeout can be completely private (for now).

annevk commented 9 years ago

@pornel yes, perhaps. If we expose termination I would expect it to be on Request myself, as we also plan to expose Request elsewhere, on elements and such.

annevk commented 9 years ago

@matthew-andrews baby steps. It'll come.

wheresrhys commented 9 years ago

Ideally we would have a design for cancelable promises that fetch could use.

What are the reasons that rejection of an ordinary promise would be an unacceptable solution?@domenic

annevk commented 9 years ago

How do you envision that would work?

wheresrhys commented 9 years ago

Assuming that fetch creates the promise, then can't it just have a setTimeout* within the executor function which calls reject? Why is that not feasible?

In the meantime I think a sensible cancelation strategy is to reject the promise with some sort of error.

- @domenic

Why is that not good enough to run with?

*or equivalent in whichever language the browser implements it in

wanderview commented 9 years ago

If this is spec'd, please also define behavior for timeouts occurring after headers are received, but before the end of the content body.

Jxck commented 9 years ago

same question here, I wanna abort() request.

I think what we need is abort(). timeout is enable using setTimeout + abort. and in the fetch spec, I think the problem is fetch doesn't has a instance, only a function. fetching process needs a internal state, and will resolve at endpoint state, reject otherwise.

in my opinion will like this.

var req = new Request(url, option);
req.fetch().then((res) => {
  console.log(res.status);
}).catch((err) => {
  console.error(err); // this will also happen in `abort()` request
});

// timeout a request
setTimeout(function() {
  req.abort(); // reject the fetching process
}, 1000);
annevk commented 9 years ago

I agree that we want something like that. It's a bit unclear to me still what the exact semantics should be.

matthew-andrews commented 9 years ago

As one possible option, node-fetch has implemented timeouthttps://github.com/bitinn/node-fetch#options — might be worth looking at/considering… it's simple :smile:

annevk commented 9 years ago

The semantics are still unclear. Does the timeout run until the promise returns? Does the timeout terminate the underlying stream somehow once the promise has returned?

Jxck commented 9 years ago

@matthew-andrews I think timeout should be Hi-level API on abort().

@annevk I think node.js http module will help us including make res as streams for getting progress event. http://nodejs.org/api/http.html#http_http_request_options_callback

kornelski commented 9 years ago

There's an interesting difference between "deadline" and "timeout".

From man ping:

The "deadline" option is implementable using setTimeout(), but an actual timeout, when measured as time from the last packet from the server, is actually more interesting and useful, and something that only browser can implement. I'd definitely like timeout to mean the latter, because it can distinguish between connection that's dead and a working connection that's just slow.

Jxck commented 9 years ago

that's interesting but is that scope of fetching ? network programming has tons of configurable options, but adding much of them will make spec like a Camel.

in fetch context, I think abort should same behave on XHR, because XHR could build upon fetch api.

in application, that will use for terminate the process gracefully, and getting back flow control to user land javascript. (ex, force reject promise, force call a callback)

If you wanna do something like that, raw-socket API may works.

kornelski commented 9 years ago

@Jxck Yes, I think timeout is in scope of fetching, and that's an important feature.

In the FT app we've had to add timeouts to every network request, because we've learned that on some networks requests just never finish, and that can make the app unusable. Now we're treating network requests without a timeout as a bug.

I'm not opposed to having an API to abort requests. I think these are largely independent problems: it's possible to have timeouts without a public abort API, and it's possible to have only abort API and leave timeout implementation to the user. I think having both would be the best.

If the decision was to only have an abort API instead of support for timeout, then that would be workable, but not ideal — I'd always have to wrap fetch in setTimeout myself, and perhaps add even more elaborate wrapper if I wanted to implement timeout from the last received bit of data, rather than a simple "deadline" option that could unnecessarily throw away data on slow connections.

Jxck commented 9 years ago

ah ok you talking about timeout option, not timeout() method. that seems to add option to RequestInit https://fetch.spec.whatwg.org/#requestinit (add or not is another discussion)

adding a option doesn't breaking changes of fetch(url, option). but adding a programmable abort() or progress event may change interface.

in some browser start to implements fetch(), so I think we need to start discussion about breaking changes immediately if we do. (or too late to change fetch api already?)

annevk commented 9 years ago

The problem with Request.prototype.abort() is that at the moment fetch() clones its input. That is, the current design is such that you can fetch() a single Request instance multiple times (unless it has a body).

(There is also some complication with Request being used as cache key and as a way to tell a service worker what a client (document/worker) wants.)

Would need to think a bit about how we can change some of these characteristics to make it all fit better.

wojciak commented 9 years ago

It depends how high of an API fetch should be. If it's a low-level request - we don't need abort. We'll wrap it in a higher level promise API and that's it.

If on the other hand it's high-level and we want to abort, it should be instantiated and work like a resource so: var someResource = new Fetch(); someResource.abort();

The first option imho is better.

annevk commented 9 years ago

Discussion for abort() moved to https://github.com/slightlyoff/ServiceWorker/issues/625 with a subclassed promise returned from fetch() being the leading candidate to expose it.

Leaving this open for the timeout idea from @pornel which seems like a rather interesting primitive to expose, network stacks allowing.

Jxck commented 9 years ago

doing fetch with just one function is difficult to expand with adding api in future. (anti Open-Closed Principle)

in current interface, we do all thing with

the problem is current fetch() is a high level api. but I believe we need a low level api for fetching what has capability for building XHR or kind of high-level api on top of it.

I heard that chrome are going to export fetch to window in M42 (mid April) If that happens, it became hard to change fetch interface. I think we need to discuss more about current api while we have a chance to change api.

annevk commented 9 years ago

What is wrong with fetch() as a low-level API? It seems to me that the amount of extensibility hooks we have is fine. We have the Request object for initialization, and at some point we'll have the FetchPromise object for status and control. What else is needed?

annevk commented 9 years ago

Also, I would appreciate it if you could move such a discussion to a new issue. Or perhaps we should move timeout to a new issue as this is getting rather cluttered.

Mouvedia commented 9 years ago

You need an xhr-parity label on this issue.

For the ones confused, we are requesting XMLHttpRequest.timeout which is either an unsigned long (Mozilla) or integer (Microsoft) representing millisecond(s).

danostrowski commented 9 years ago

As @wheresrhys and @pornel have pointed out, it seems very trivial to pass along a timeout option and add an ontimeout handler that calls reject in the construction of the Promise returned by fetch.

For people moving from jQuery.ajax and the like, this is a barrier that should be easy to fix. Should we just submit a patch?

annevk commented 9 years ago

It's not actually that easy, given CORS preflights, HTTP authentication, involvement of service workers, and the choice mentioned in https://github.com/whatwg/fetch/issues/20#issuecomment-73727547. Also, if we get abort semantics, we might want to use those here and not reject, so that is unclear too at this point.

danostrowski commented 9 years ago

Hmmm. Interesting.

XMLHttpRequests work with CORS preflights and HTTP authentication, right? Why would this be any different?

I confess I'm not familiar with how "involvement of service workers" affects this, is there some comment that explains what negative interaction might happen with timeout? The mechanism for handling .ontimeout seems the same as .onload in the context of the main .fetch operation.

Regarding comment #20, is there a compelling reason to make timeout behave any differently from how it's already defined on XHR? This isn't ping; seems like a non sequitur.

Also, if we get abort semantics, we might want to use those here and not reject, so that is unclear too at this point.

You mean... change the object you get out of .fetch to not be a standard JavaScript Promise?

Krinkle commented 9 years ago

@danostrowski How to abort a request is relatively straight forward. (Or at least outside the scope of this issue).

Unfortunately, it seems we're blocked on figuring out a way to expose this feature. E.g. do we add an .abort() method to the Promise returned by fetch()? And how does this interact with other promises after calling .then(). See whatwg/fetch#27.

danostrowski commented 9 years ago

I wasn't proposing an abort, I don't see it as a necessity, from the outside. A timeout argument to xhr suffices which simply results in .reject being called which, in turn, works automatically with the Promise methodology.

kornelski commented 9 years ago

@Krinkle I don't want abort() or any other API to abort requests externally. What I need is rejection of the promise if the network is unusable or stalled. This is a different thing.

Basically this, but a bit better implemented:

const oldfetch = fetch;
fetch = function(input, opts) {
    return new Promise((resolve, reject) => {
        setTimeout(reject, opts.deadline);
        oldfetch(input, opts).then(resolve, reject);
    });
}
jonathan-fielding commented 9 years ago

@pornel I agree with your example, we need to be able to pass a timeout as part of the options and then the error passed to the rejected promise should tell us that the timeout was exceeded.

Would also be useful to be able to timeout, do something to handle timeout condition and then if the response eventually comes back to be able to resume (kinda like when you regain a connection perhaps). Not sure if this is asking a bit to much from fetch API though.

peteruithoven commented 9 years ago

Any progress on this?

anka-213 commented 8 years ago

What is blocking this issue? As far as I can see, this is not dependent on #27 (which is obviously not going to be solved in the near future). All that should be needed is an option for timeout and to reject the promise on timeout. That doesn't seem controversial.

DiThi commented 8 years ago

I subscribed to this issue time ago because I need connections to fail when they're stuck. However, it's common to have many requests pending because the browser only downloads a limited number simultaneously. Therefore we need a way to timeout a whole group of promises when there hasn't been activity for X amount of time.

I've been using the timeout property of XHR but it turns out it's a timeout since send() was called, which is like setting a setTimeout regardless of traffic. I had to remove the timeout because it always failed on slow connections.

In other words, we need several things, independently:

annevk commented 8 years ago

@DiThi note that XMLHttpRequest does offer that ability since you can keep increasing the timeout as data arrives. Whatever we do here though does depend on #27 and corresponding questions for streams, since they all interact.

kornelski commented 8 years ago

Whatever we do here though does depend on #27

No, it does not. "Provide developers a method to abort" is very different than "implementation can internally abort and reject the promise without giving developers any other API to abort".

What about https://github.com/whatwg/fetch/issues/20#issuecomment-135426968 ?

annevk commented 8 years ago

So you only want the timeout to take effect for the period of time until all HTTP headers have been received? Any network slowness after that is not important?

kornelski commented 8 years ago

Yes, for an "MVP" that would be useful already, as in my experience most stuck connections happen before server response.

kornelski commented 8 years ago

Ideally I'd like the timeout to apply to streams too (and be counted since last data received), but I'd rather have something sooner, rather than wait another year on debate of another API that I wouldn't use.

In the codebase I'm working on right now I've got 33 uses of request timeout, 0 uses of request abort.

DiThi commented 8 years ago

@annevk I did exactly that increasing the timeout after an error. But imagine a file that takes more than a minute to be downloaded in a slow connection, with a minute of timeout. It ends taking twice as much time and bandwith! (even more when the timeout was half a minute).

More than abort, we need progress. Having both we can detect stuck connections and restart them. Having just abort we can easily emulate the XHR timeout feature (to avoid downloading the same data several times).

In other words: We need first a way to measure download progress. Then a way to abort the connection. Not necessarily exposed to JS, but just usable by the timeout and deadline implementations.

Maybe we should split the issue in two? Timeout for stuck connections (not just slow ones) and deadline (if it's not downloaded in X time, I don't need it; a.k.a. XHR timeout).

jakearchibald commented 8 years ago

We should offer lower-level APIs so more combinations can be composed.

We already have a time api - setTimeout. Streams will give you insight into download progress (although we may want to consider how uploads are observed, especially if redirects happen). Abort is the other missing piece.

kornelski commented 8 years ago

Yes, I think splitting the issue into a simple deadline and more advanced use cases would help, because currently the discussion looks like:

Done: #179 #180

alcat2008 commented 8 years ago

I think the code below would be helpful.

var p = Promise.race([
  fetch('/resource-that-may-take-a-while'),
  new Promise(function (resolve, reject) {
    setTimeout(() => reject(new Error('request timeout')), 5000)
  })
])
p.then(response => console.log(response))
p.catch(error => console.log(error))
DiThi commented 8 years ago

@alcat2008 That code is very useful, thanks.

However it fails to avoid data waste when it hasn't started yet (#179) or when it's a big file that is expected to take a while to finish (#180). Both things happened to me frequently over an ADSL connection.

Since two separate issues were made and the solution outside these two is just to use race(), I propose to close this one.

buraktamturk commented 8 years ago

Something like this could allow us to cancel requests manually (without timeout).


var cancelpromise1 = new Promise(function(resolve, reject) {
     setTimeout(resolve, 1000);
});

var cancelpromise2 = /* a promise that will be resolved when user clicks cancel */;

var lastpromise = Promise.race([cancelpromise1, cancelpromise2]);

fetch(..., { timeout: lastpromise })
   ...
CamiloTerevinto commented 7 years ago

It's been well over an year since the last comment, is there any progress on any of this?

annevk commented 7 years ago

We're pretty close to fixing #447. That should make it a lot easier to implement this in a library and eventually get something standardized.

jakearchibald commented 7 years ago

Using the abort syntax, you'll be able to do:

const controller = new AbortController();
const signal = controller.signal;

const fetchPromise = fetch(url, {signal});

// 5 second timeout:
const timeoutId = setTimeout(() => controller.abort(), 5000);
const response = await fetchPromise;
// …

If you only wanted to timeout the request, not the response, add:

clearTimeout(timeoutId);
// …
mattlubner commented 7 years ago

@jakearchibald Forgive me if I'm being obtuse, but how does clearing the timeout after the response resolves timeout only the request? If clearTimeout(timeoutId) is after await fetchPromise, it will only clear the timeout after the response is received. Unless I'm missing something?