hyper icon indicating copy to clipboard operation
hyper copied to clipboard

feat(server): add h1 `idle_timeout`

Open finnbear opened this issue 1 year ago • 2 comments

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

  • [x] Added idle_timeout setting, optimized to avoid creating an additional timer per request
  • [ ] ~~Automatically send keep-alive: timeout=# header~~ (deferred due to: https://github.com/hyperium/http/issues/142)

Testing

  • [x] Fixed multiple pre-existing and broken, but relevant, tests
  • [x] Added a test with idle timeout
  • [x] Added a test without idle timeout (improves coverage)
  • [x] 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

  • [x] Support in hyper-util (https://github.com/hyperium/hyper-util/pull/146)
  • [ ] Consider other timeouts (reading body, writing response)

finnbear avatar Aug 24 '24 06:08 finnbear

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

MarkusSintonen avatar Oct 05 '24 10:10 MarkusSintonen

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 :)

finnbear avatar Oct 05 '24 17:10 finnbear

~~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~~ see next comment

~~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 avatar Jan 01 '25 04:01 finnbear

It seems I confused myself by forgetting the details of my changes. Unmodified hyper does not "skip to step 2." That requires a simple modification, which was part of this PR: image

See #3828, which supersedes this PR

finnbear avatar Jan 14 '25 18:01 finnbear