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

feat: change timeouts to 120 seconds #1384

Closed
wants to merge 2 commits into from
Closed

feat: change timeouts to 120 seconds #1384

wants to merge 2 commits into from

Conversation

kyrylkov
Copy link
Contributor

No description provided.

@kyrylkov kyrylkov changed the title Change timeouts to 120 seconds feat: change timeouts to 120 seconds Apr 26, 2022
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

We will ship this when we cut a new major.

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If prefer if we only change this for fetch.

@kyrylkov
Copy link
Contributor Author

kyrylkov commented Apr 27, 2022

@ronag I looked through fetch code but I wasn't able to figure out how to change timeouts there. Also for server requests 30 seconds is just too short. We deal with lots of long running requests and that's how we ran into this problem in the first place.

@ronag
Copy link
Member

ronag commented Apr 27, 2022

@ronag I looked through fetch code but I wasn't able to figure out how to change timeouts there.

You can pass timeout in the .dispatch call in the fetch code.

Also for server requests 30 seconds is just too short. We deal with lots of long running requests and that's how we ran into this problem in the first place.

I disagree and would prefer the 30 second. Maybe you could change my mind but I would need some more convincing.

@kyrylkov
Copy link
Contributor Author

I disagree and would prefer the 30 second. Maybe you could change my mind but I would need some more convincing.

30 second timeout exists only in Firefox. Node default timeout is 120. Chrome is 300.

Otherwise why 30? Why not 10? There should be some consistency. Aligning with Node or Chrome seems like a good idea.

@ronag
Copy link
Member

ronag commented Apr 27, 2022

Node default timeout is 120.

As far as I can see from the code and docs there is no default timeout for Node?

To be clear I'm fine with changing the timeout for fetch. Just not undici in general as I think 30s is a much more useful timeout for a server workload. Changing the timeout for fetch is something we can land without semver major.

@kyrylkov
Copy link
Contributor Author

@ronag Then how about another PR for fetch timeout for 300 seconds (like in Chrome)?

@ronag
Copy link
Member

ronag commented Apr 27, 2022

@ronag Then how about another PR for fetch timeout for 300 seconds (like in Chrome)?

SGTM!

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants