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 configure ReadHeaderTimeout for HTTP server #423

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

pstibrany
Copy link
Member

@pstibrany pstibrany commented Oct 30, 2023

What this PR does: Set read header timeout explicitly. This allows setting of shorter timeout for headers, and not using entire -server.http-read-timeout.

Checklist

  • [na] Tests updated -- Did not add tests, because it would be testing documented behaviour of Golang's HTTP server.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

@pstibrany pstibrany marked this pull request as ready for review October 30, 2023 10:59
IdleTimeout: cfg.HTTPServerIdleTimeout,
Handler: middleware.Merge(httpMiddleware...).Wrap(router),
ReadTimeout: cfg.HTTPServerReadTimeout,
ReadHeaderTimeout: cfg.HTTPServerReadHeaderTimeout,
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a comment to explain why we want to set the read header timeout.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL to see if new descriptions make sense.

@pstibrany pstibrany marked this pull request as draft October 30, 2023 11:08
@pstibrany
Copy link
Member Author

pstibrany commented Oct 30, 2023

Converted to draft again. As it is now, the field descriptions and CHANGELOG entry is wrong.

Image explaining the timeouts:

@pstibrany
Copy link
Member Author

Converted to draft again. As it is now, PR is wrong. (Mostly documentation).

Updated the description of fields and changelog entry.

@pstibrany pstibrany marked this pull request as ready for review October 30, 2023 11:22
@pstibrany pstibrany merged commit cd0341d into main Oct 30, 2023
3 checks passed
@pstibrany pstibrany deleted the read-header-timeout branch October 30, 2023 14:39
@pstibrany pstibrany changed the title Use ReadHeaderTimeout Add option to configure ReadHeaderTimeout for HTTP server Oct 30, 2023
ying-jeanne pushed a commit that referenced this pull request Nov 2, 2023
* By setting ReadHeaderTimeout, ReadTimeout is only used for body.

* Add config option for read header timeout.

* Add CHANGELOG.md entry.

* Fix documentation and changelog entry.
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.

None yet

2 participants