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 option to reject the fetch promise automatically after a certain time elapsed (with no API for arbitrary aborts) #179

Open
kornelski opened this issue Dec 14, 2015 · 23 comments
Labels
addition/proposal New features or enhancements needs implementer interest Moving the issue forward requires implementers to express interest topic: api

Comments

@kornelski
Copy link

kornelski commented Dec 14, 2015

I'd like an option to have a timeout/deadline for the server to respond, so that if the response doesn't arrive within a specified number of seconds, the fetch promise is rejected and internally (i.e. without any sort of abort/cancellation API exposed to JS) the request is aborted by the browser on the TCP/IP level, so that bandwidth is not wasted in case the response does come later.

fetch('/slow', {deadline: 5000})
  .then(called_if_response_is_quick, called_if_response_is_too_slow);

It'd be less wasteful equivalent of:

const oldfetch = fetch;
fetch = function(input, opts) {
    return new Promise((resolve, reject) => {
        setTimeout(reject, opts.deadline);
        oldfetch(input, opts).then(resolve, reject);
    });
}

I wouldn't mind if the timeout was automagically propagated to response.json(), so that promise would be rejected too after the deadline.

Use-cases:

  • Overloaded web servers. Servers may accept connections quickly (e.g. by a reverse proxy or incoming-connection worker thread) even if they don't have "back-end" capacity to process the request, so requests are getting queued and responses won't be generated in a timely fashion.
  • Mobile connections with a very weak signal. Tethering via WiFi to a mobile device with no or weak signal.
    In such cases it seems that the browser is unaware that it has an unusable network connection, makes a request and never gives up waiting for a response (perhaps the request was successfully sent or at least was buffered somewhere along the way).

In such cases adding a deadline helps identify unusable network or servers quicker (the developer knows how fast the server is supposed to respond) and allows the application react to the situation rather than getting stuck forever (or for as long as very conservative deadlines are built into the browser or networking stack) or worse, having open and unusable connections exhaust browser's limit of open connections.

@aronwoost

This comment has been minimized.

2 similar comments
@joshmangum

This comment has been minimized.

@rogeriochaves

This comment has been minimized.

@Mouvedia
Copy link

FYI this corresponds to the behaviour of XHR timeout. Which is what #20 is requesting.

@buraktamturk
Copy link

Something like this just better than just specifying integer for timeout parameter. Manually cancelling the request is a thing.

#20 (comment)

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 })
   ...

@danostrowski

This comment has been minimized.

@isijara

This comment has been minimized.

@CamiloTerevinto

This comment has been minimized.

@jakearchibald

This comment has been minimized.

@baybal

This comment has been minimized.

@annevk annevk added the needs implementer interest Moving the issue forward requires implementers to express interest label Jan 2, 2019
@annevk
Copy link
Member

annevk commented Jan 2, 2019

It would help though if this was motivated a bit beyond "less wasteful". Is this primarily about ergonomics? Is it different from XMLHttpRequest's timeout?

@kornelski
Copy link
Author

kornelski commented Jan 2, 2019

What I'm proposing is functionally the same as XHR's timeout. Ergonomics is a very important part of it, because for a robust web app the timeout has to be added to every single fetch() call (I'm serious).

I know this from experience of developing a major PWA. We've found that there are unbelievably broken phones/mobile operators that made connections appear infinitely slow (similar to what @jakearchibald calls Lie-Fi) so our requests would get stuck waiting forever, and in turn keep related bits of logic or UI similarly stuck all over the place.

I have fixed this by adding timeouts to our XHR calls, and contributed such interface to superagent.

@annevk
Copy link
Member

annevk commented Jan 2, 2019

To be clear, I was not trying to dismiss the ergonomics perspective. Mostly making sure I understood the technical requirements. I think I'm supportive of adding this as a shorthand for calling https://fetch.spec.whatwg.org/#abort-fetch. I'd suggest we call it timeout for continuity with XMLHttpRequest (we also reuse status, statusText, and such).

The main thing we lack is active implementer interest and folks to make all necessary changes (PR Fetch, PR WPT).

@Mouvedia
Copy link

Mouvedia commented Jan 2, 2019

I think I'm supportive of adding this as a shorthand for calling https://fetch.spec.whatwg.org/#abort-fetch.

That's great, but it should remain internal, i.e. it should trigger ontimeout not onabort.

@jchook
Copy link

jchook commented Jan 25, 2019

I use this, based on @buraktamturk's suggestion.

const networkTimeoutError = new Error('Network request timeout')

function getTimeout({ timeout }) {
  if (timeout && timeout.constructor === Promise) {
    return timeout
  }
  if (typeof timeout === 'number') {
    return new Promise(resolve => {
      setTimeout(resolve, timeout)
    })
  }
}

export default function fetchWithTimeout(url, config) {
  return new Promise((resolve, reject) => {
    let completed = false
    const timeout = getTimeout(config)
    const wrap = (thing, ...pre) => (...args) => {
      if (!completed) {
        completed = true
        return thing(...pre, ...args)
      }
    }
    delete config.timeout
    if (timeout) {
      timeout.then(wrap(reject, networkTimeoutError))
    }
    return fetch(url, config)
      .then(wrap(resolve))
      .catch(wrap(reject))
  })
}

@annevk
Copy link
Member

annevk commented Oct 14, 2019

Does the caller need to be able to distinguish between cancelation and timing out? XMLHttpRequest provides that distinction, but something like AbortSignal.timeout(ms) would not.

@ianstormtaylor
Copy link

ianstormtaylor commented Oct 14, 2019

I had totally missed this issue, and wrote up some arguments in #951.

I agree that the ergonomics are paramount (to cover the 90% case) because literally every fetch call should have an associated timeout. Python's requests docs cover it well:

You can tell Requests to stop waiting for a response after a given number of seconds with the timeout parameter. Nearly all production code should use this parameter in nearly all requests. Failure to do so can cause your program to hang indefinitely. Source

To eliminate the hanging failure case the timeout timer needs to extend to cover .json() (and other body-reading promises) as well. (A lot of the existing examples that use promise-based timeouts do not handle this case and are buggy as written.)

This is similar to what Go does for their Timeout parameter:

Timeout specifies a time limit for requests made by this Client. The timeout includes connection time, any redirects, and reading the response body. The timer remains running after Get, Head, Post, or Do return and will interrupt reading of the Response.Body. Source

Thanks!

@annevk
Copy link
Member

annevk commented Feb 25, 2021

I'm still interested in an answer to the question I raised above as AbortSignal.timeout is being proposed (again): whatwg/dom#951.

One high-level question I have for @kornelski is that although it was said this would be equivalent to XMLHttpRequest, the way OP reads to me is that we'd wait up to X seconds for the first chunk and after that the timeout no longer applies, whereas XMLHttpRequest timeout (and aborting) continues affecting subsequent chunks.

@kornelski
Copy link
Author

kornelski commented Feb 25, 2021

Indeed, I've mixed up two concerns:

  1. Externally timing out via promise/setTimeout wasn't aborting the underlying network request, unlike timing out via XHR.

  2. I wanted it to be even smarter than XHR to support timeout since the last received bit of data, rather than since start of the request.

I think case 1 is solved. For case 2 I've written my XHR-based client that split timeout into timeout for the initial response (http headers) and timeout for the whole body. That ended up being close enough to solve the problem of distinguishing working slow connections from not getting any server response.

Anyway, I'm not working any more on the project that motivated this feature request, so I don't have up to date information whether this is still needed.

@annevk
Copy link
Member

annevk commented Feb 25, 2021

Well, you did also file #180 and maybe we'll get signals for individual read operations on a stream which would address that case on a per-chunk basis.

And I guess timeout for response, but not response's body, can be implemented in userland as well because you can prevent calling abort() once you have a response, though this would require some tight coordination between the signal and the operation.

@benjamingr
Copy link
Member

Happened to find this but I think this can be closed now given we have AbortSignal and timeouts

@benjamingr
Copy link
Member

Actually just saw Anne's comment about clarification about which timeout this is:

the way OP reads to me is that we'd wait up to X seconds for the first chunk and after that the timeout no longer applies, whereas XMLHttpRequest timeout (and aborting) continues affecting subsequent chunks.

@benjamingr benjamingr reopened this Aug 3, 2022
@josephrocca
Copy link

Actually just saw Anne's comment

My primary concern at this point is the ergonomics (readability and writeability) of this:

fetch(url, { signal: AbortSignal.timeout(8000) });

Semantically convoluted on a skim, and just significantly longer than it needs to be for 99% of cases. We need to add this on basically every request. So I'm of the very strong opinion that this should not be closed until we have something ergonomically ~equivalent to:

fetch(url, { timeout: 8000 });

And probably handled in a similar way to Go, as mentioned by ianstormtaylor in an earlier comment in this thread.

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