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

Add timeout option #20

Closed
wheresrhys opened this issue Jan 15, 2015 · 104 comments
Closed

Add timeout option #20

wheresrhys opened this issue Jan 15, 2015 · 104 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api

Comments

@wheresrhys
Copy link

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.

JakeChampion/fetch#68

@domenic
Copy link
Member

domenic commented Jan 15, 2015

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:

  • 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().
  • Come up with something more awkward. E.g., fetch.cancelable(...) returns a { promise, cancel } tuple. Not horrible with destructuring but pretty awkward. And will likely be obsoleted by cancelable promises.

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
Copy link

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
Copy link

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
Copy link
Member

annevk commented Jan 15, 2015

@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
Copy link
Member

annevk commented Jan 15, 2015

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

@wheresrhys
Copy link
Author

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
Copy link
Member

annevk commented Jan 16, 2015

How do you envision that would work?

@wheresrhys
Copy link
Author

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
Copy link
Member

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
Copy link

Jxck commented Feb 10, 2015

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
Copy link
Member

annevk commented Feb 10, 2015

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

@matthew-andrews
Copy link

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

@annevk
Copy link
Member

annevk commented Feb 10, 2015

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
Copy link

Jxck commented Feb 10, 2015

@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
Copy link

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

From man ping:

  • -w deadline
    Specify a timeout, in seconds, before ping exits regardless of how many packets have been sent or received. In this case ping does not stop after count packet are sent, it waits either for deadline expire or until count probes are answered or for some error
    notification from network.
  • -W timeout
    Time to wait for a response, in seconds. The option affects only timeout in absense of any responses, otherwise ping waits for two RTTs.

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
Copy link

Jxck commented Feb 10, 2015

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
Copy link

@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
Copy link

Jxck commented Feb 11, 2015

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
Copy link
Member

annevk commented Feb 13, 2015

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
Copy link

wojciak commented Feb 13, 2015

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
Copy link
Member

annevk commented Feb 17, 2015

Discussion for abort() moved to w3c/ServiceWorker#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
Copy link

Jxck commented Feb 26, 2015

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

  • adding new props to option arguments
  • adding new methods to returned promise

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
Copy link
Member

annevk commented Feb 26, 2015

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
Copy link
Member

annevk commented Feb 26, 2015

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.

@jokeyrhyme
Copy link

jokeyrhyme commented Oct 12, 2019

There's a potentially different kind of timeout here: https://aws.amazon.com/blogs/aws/elb-idle-timeout-control/
An "idle timeout", for when there is no activity on a connection for X seconds

e.g. with an idle timeout of 15 seconds:

  • a slow connection that trickles out the headers at 10 seconds and a body chunk every 10 seconds will NOT trigger the idle timeout
  • a non-streaming server that returns the headers at 1 second, but takes 20 seconds to render the body WILL trigger the idle timeout
  • a cellular connection on a train with headers at 1 second, some of the body at 10 seconds, then a 20 second tunnel WILL trigger the idle timeout

I don't think it is always the intention for a timeout to only consider the time until the first byte

I don't think it is always the intention for a timeout to consider full receipt of the headers and body

But each of these ways of defining timeouts is useful in different situations

@Pauan
Copy link

Pauan commented Oct 12, 2019

@Mouvedia As the spec clearly shows (in step 14), XHR will wait for both the headers and the body to complete. The timeout for XHR includes the body. Please read the spec more carefully.

XHR has readystate 3 which corresponds to HEADERS_RECEIVED. We gotta strive for parity with XHR.

The load event will only fire when the body has been received. The timeout cares about the load event, not readystate.

XHR is not the same as fetch. XHR always retrieves the body, whereas fetch lets you choose whether to retrieve the body or not. That's why fetch needs to have a choice whether the timeout includes the body or not.


@jokeyrhyme That's a good point, that complicates the timeout even more. Which is another argument for having this in user-land, so that way the user has full control, rather than adding in several options to fetch.

@FranklinYu
Copy link

If the "server hangs" and it happens during the streaming, you won't receive another chunk.

@Mouvedia Yes, you won't receive another chunk, but the promise of .text() won't resolve either. It simply wait for the server to recover (possibly up until the network/system timeout which could be 5 minutes or similar).

@FranklinYu
Copy link

  • a slow connection that trickles out the headers at 10 seconds and a body chunk every 10 seconds will NOT trigger the idle timeout

@jokeyrhyme I doubt whether this can be detected in JavaScript. Now that the body is a stream, is browser allowed to buffer the response? Is browser supposed to trigger any JavaScript event as soon as it receive a single byte from server?

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Oct 14, 2019
@ImanMh

This comment has been minimized.

yutakahirano added a commit that referenced this issue Jun 23, 2020
# This is the 1st commit message:

# This is a combination of 23 commits.
# This is the 1st commit message:

Integrate CORP and COEP

This is part of the introduction of COEP
(whatwg/html#5454). The CORP check now takes
COEP into account. Also, responses coming from service workers
are checked.

# This is the commit message #2:

Update fetch.bs

Co-authored-by: Domenic Denicola <[email protected]>
# This is the commit message #3:

Update fetch.bs

Co-authored-by: Domenic Denicola <[email protected]>
# This is the commit message #4:

fix

# This is the commit message #5:

fix

# This is the commit message #6:

fix

# This is the commit message #7:

fix

# This is the commit message #8:

fix

# This is the commit message #9:

fix

# This is the commit message #10:

fix

# This is the commit message #11:

fix

# This is the commit message #12:

fix

# This is the commit message #13:

fix

# This is the commit message #14:

fix

# This is the commit message #15:

fix

# This is the commit message #16:

fix

# This is the commit message #17:

fix

# This is the commit message #18:

Update fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>
# This is the commit message #19:

Update fetch.bs

Co-authored-by: Anne van Kesteren <[email protected]>
# This is the commit message #20:

fix

# This is the commit message #21:

fix

# This is the commit message #22:

fix

# This is the commit message #23:

fix

# This is the commit message #2:

fix
@annevk
Copy link
Member

annevk commented Feb 25, 2021

I noticed some misunderstanding about fetch() above. It will retrieve the body, just like XMLHttpRequest. So timeout would work similarly.

We're considering adding timeout support to AbortSignal: whatwg/dom#951. What I'm wondering is whether people need to distinguish between the different reasons a fetch might have aborted or whether that's immaterial.

@Mouvedia
Copy link

Mouvedia commented Feb 25, 2021

What's annoying is non-parity with XHR: fetch will call the signal's abort handler even if the request has been completed—XHR, as expected, doesn't. After the completion the callback should become a no-op.

@annevk
Copy link
Member

annevk commented Feb 25, 2021

That doesn't make sense. You pass a signal to fetch() and that will react to the abort signal. If the fetch has completed it'll no-op.

@Mouvedia
Copy link

<off topic>

If the fetch has completed it'll no-op.

Are you talking about the specification or the implementations? Do you have tests for that?
</off topic>


Anyway what I meant is, please try to strive for parity with XHR. We don't want more gotchas: the specification needs to be explicit. Ill give you another example, in XHR abortion behaviour on page quit wasn't clear at first.
So first and foremost parity, then you can move on and add new functionalities that are missing.
i.e. don't reactively change how XHR behaves in light of what you are trying to add to fetch (because XHR is now explained in terms of fetch)

@annevk
Copy link
Member

annevk commented Feb 25, 2021

I'm not sure I follow. There are open issues around document unloading, but they affect XMLHttpRequest and fetch() equally. And I wasn't even talking about adding anything to fetch here...

@Mouvedia
Copy link

And I wasn't even talking about adding anything to fetch here...

From the point of view of developers, adding timeout to AbortSignal is directly related to XHR obviously.

We're considering adding timeout support to AbortSignal: whatwg/dom#951.

Are you adding timeout or deadline?

@annevk
Copy link
Member

annevk commented Feb 25, 2021

It's on top of signal, so deadline, which is also what XMLHttpRequest has. However, we might add signals to reading from a stream, which would allow for the former.

@Mouvedia
Copy link

Dunno what the others think but it would be better if we could stay consistent regarding terminology.
Personally I don't care what it is called as long as I have at least XHR parity.

@ririko5834
Copy link

How can I add timeout to request now?

@saschanaz
Copy link
Member

Should this be considered fixed as we now have AbortSignal.timeout?

@annevk
Copy link
Member

annevk commented May 23, 2022

Thanks, yeah, this can be closed. We still have #179 and #180 for the more complicated scenarios.

@annevk annevk closed this as completed May 23, 2022
@karlhorky
Copy link

karlhorky commented May 27, 2022

AbortSignal.timeout looks great, thanks to everyone who worked on speccing and implementing this! (whatwg/dom#1032, w3ctag/design-reviews#711)

// Fetch the URL, cancelling after 8 seconds
fetch(url, { signal: AbortSignal.timeout(8000) });

Apparently available in Firefox 100 and Chrome 103 beta!!

@saschanaz
Copy link
Member

And in Firefox 100 😉

@karlhorky
Copy link

Thanks, wasn't aware about this, updated my post above!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api
Development

No branches or pull requests