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

HTTP Setup: Default WriteTimeout #4785

Open
C0x41lch0x41 opened this issue Mar 3, 2023 · 2 comments · May be fixed by #4796
Open

HTTP Setup: Default WriteTimeout #4785

C0x41lch0x41 opened this issue Mar 3, 2023 · 2 comments · May be fixed by #4796

Comments

@C0x41lch0x41
Copy link
Contributor

What problem does your feature solve?

There is no default WriteTimeout in the setup of http server. If the server instance is used using a configuration without WriteTimeout it will be set to 0 which means no timeout. That may be an issue if a request takes too much time to process and can potentially lead to DoS the server.
We need to be careful how this value is set though. It should be more than ReadTimeout.

Those two articles provide useful information in my opinion:

What would you like to see?

A default value for WriteTimeout in HTTP setup function

What alternatives are there?

We already have a default ReadTimeout and Go >= 1.13 sets a pretty aggressive TCP timeout, so this may be sufficient if our services does not let the client control parameters that influence the processing time of a request. But it is good practice in case there is a edge case.

@mollykarcher mollykarcher moved this from Backlog to In Progress in Platform Scrum Mar 9, 2023
@stellarsaur stellarsaur moved this from In Progress to Backlog in Platform Scrum May 23, 2023
@stellarsaur
Copy link
Contributor

We discussed this, and came to the conclusion we need to take a more careful look at our existing timeouts to make sure we use the right default WriteTimeout. Ideally, this should be longer than all our existing timeouts, but not unlimited like it currently is.

Moved back to our backlog for now.

@leighmcculloch
Copy link
Member

Fyi this was something we investigated previously. There are some findings here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging a pull request may close this issue.

3 participants