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(server): add h1 idle_timeout #3743

Closed
wants to merge 2 commits into from

Conversation

finnbear
Copy link

@finnbear finnbear commented Aug 24, 2024

Motivation

Older browsers sometimes open 6 or more H1 connections and keep them alive and idle after resources are loaded, costing server resources and distracting from DoS attackers. In my specific case, DoS mitigation required limiting TCP connections per IP address, but limits had to be high to account for multiple legitimate H1 browsers on the same IP address.

Related: #1628
Related: #2355

Changes

Testing

  • Fixed multiple pre-existing and broken, but relevant, tests
  • Added a test with idle timeout
  • Added a test without idle timeout (improves coverage)
  • Tested in my app
# Cargo.toml

[patch.crates-io]
hyper = { git = "https://github.com/finnbear/hyper", branch = "idle_timeout" }
hyper-util = { git = "https://github.com/finnbear/hyper-util", branch = "idle_timeout" }

Future work

@finnbear finnbear marked this pull request as ready for review August 24, 2024 07:19
@finnbear finnbear marked this pull request as draft August 24, 2024 18:45
@finnbear finnbear marked this pull request as ready for review August 24, 2024 19:29
@MarkusSintonen
Copy link

@sfackler / @seanmonstar is there a possibility on getting this PR reviewed? I am also interested in getting H1 idle timeouts available :)

@finnbear
Copy link
Author

finnbear commented Oct 5, 2024

I am also interested in getting H1 idle timeouts available :)

If you want, you can use the TOML snippet in my PR description to avoid needing to wait for review :)

Dav1dde added a commit to getsentry/relay that referenced this pull request Nov 14, 2024
With the idle time configurable we can prevent a pile up of open
connections which never see any activity.

See also on the hyper issue tracker:
```
hyperium/hyper#3743
hyperium/hyper#1628
hyperium/hyper#2355
hyperium/hyper#2827
```
@finnbear
Copy link
Author

finnbear commented Jan 1, 2025

Today I investigated further and reached the conclusion that this PR is unnecessary for my use-case because, contrary to my prior belief, header_read_timeout is applied to all requests, not just the first one. I'm not sure why I didn't realize this before.

In other words, unmodified hyper works like this:

  1. accept connection
  2. start header read timeout
  3. read header
  4. stop header read timeout
  5. read remainder of request and write response
  6. skip to step 2

This PR would only be relevant for users who want to set different values for header read timeout and idle timeout. Any such user is welcome to:

  • explain their use-case.
  • resurrect this PR, possibly with modifications.
  • if idle_timeout < header_read_timeout, make a custom stream type and use it with header_read_timeout.
  • if idle_timeout > header_read_timeout, make a similar custom stream type, disable header_read_timeout, and reproduce the behavior of header_read_timeout by communicating with a middleware.

Other timeouts, like reading the body and writing the response, could also be added outside of hyper, by wrapping the request or response body with a timeout detector, and replacing the response with an error response with a Connection: close header.

Considering the above, I will close this PR. I intend to open a new PR that only includes my fixes to the existing test cases.

@finnbear finnbear closed this Jan 1, 2025
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.

2 participants