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

Timeout implementation for requests should be re-thought #1654

Open
flevi29 opened this issue May 19, 2024 · 0 comments · May be fixed by #1741
Open

Timeout implementation for requests should be re-thought #1654

flevi29 opened this issue May 19, 2024 · 0 comments · May be fixed by #1741
Labels
enhancement New feature or request

Comments

@flevi29
Copy link
Collaborator

flevi29 commented May 19, 2024

Currently timeout is done with a Promise.race, where we have two promises, one for the fetch and one for a setTimeout.
Using this solution will leave the request hanging and still consume bandwidth in the background and lower the maximum allowed concurrent requests being made while it's still in process. (https://stackoverflow.com/a/50101022)

async fetchWithTimeout(
url: string,
options: Record<string, any> | RequestInit | undefined,
timeout: HttpRequests['requestTimeout']
): Promise<Response> {
return new Promise((resolve, reject) => {
const fetchFn = this.httpClient ? this.httpClient : fetch
const fetchPromise = fetchFn(url, options)
const promises: Array<Promise<any>> = [fetchPromise]
// TimeoutPromise will not run if undefined or zero
let timeoutId: ReturnType<typeof setTimeout>
if (timeout) {
const timeoutPromise = new Promise((_, reject) => {
timeoutId = setTimeout(() => {
reject(new Error('Error: Request Timed Out'))
}, timeout)
})
promises.push(timeoutPromise)
}
Promise.race(promises)
.then(resolve)
.catch(reject)
.finally(() => {
clearTimeout(timeoutId)
})
})
}

Instead this timeout option should add an AbortSignal.timeout to the Request option's signal, if there isn't already one supplied. We should warn people, that if they're supplying this signal option themselves, their timeout will not work.

We could use AbortSignal.any to keep the same functionality as before, but it is not available in Node.js 18, and there's no polyfill for it either.

EDIT: Looks like AbortSignal.timeout has no polyfill either. In fact all of AbortController and AbortSignal isn't very well polyfilled, if at all. This would definitely have a big impact on compatibility, at least if one wishes to set a global timeout :\ . I really hate how the web is held back like this.

@curquiza curquiza added the enhancement New feature or request label May 20, 2024
@flevi29 flevi29 linked a pull request Oct 10, 2024 that will close this issue
3 tasks
@flevi29 flevi29 linked a pull request Oct 10, 2024 that will close this issue
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants