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

fix(server): start h1 header read timeout when conn is idle, a de facto idle timeout #3828

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

finnbear
Copy link
Contributor

@finnbear finnbear commented Jan 14, 2025

Motivation

Currently, the header read timeout is started before any part of the first request is received. This allows closing the connection if no requests are received. However, after the first request, the connection can remain open indefinitely. This change ensures that the header read timeout is started immediately after the connection is idle, following the transmission of the response, before the first part of the subsequent request is received.

This is particularly relevant in the case that browsers open 6+ h1 connections when a page loads and then neglect to close any of them, distracting, in my case, from DDoS attackers.

This is kind of like an "idle timeout"

Changes

  • Start header_read_timeout when waiting for subsequent requests
  • Error::is_timeout returns true on HeaderTimeout

Tests

  • Fix existing tests, which failed due to invalid headers, not timeouts
    • header_read_timeout_slow_writes
    • header_read_timeout_slow_writes_multiple_requests
  • Test new functionality
    • I've manually confirmed that the new test fails without the change

Open question

Should the documentation of header_read_timeout change? It currently doesn't say when the timeout is started, only when it finishes (client transmits entire header).

Related

Related: #1628
Related/Fixes: #2355
Related #3185 (comment)
Builds on and supersedes/Closes #3781 (credit to @T-aian)
Fixes #3780
Supersedes #3743 (this PR required additional complexity, but allowed a different value for the header read timeout and the idle timeout)

@finnbear finnbear changed the title feat(server): Universal h1 header read timeout. feat(server): Start h1 header read timeout when conn is idle, a de facto idle timeout. Jan 14, 2025
@finnbear finnbear marked this pull request as ready for review January 14, 2025 19:03
@finnbear finnbear changed the title feat(server): Start h1 header read timeout when conn is idle, a de facto idle timeout. feat(server): start h1 header read timeout when conn is idle, a de facto idle timeout. Jan 14, 2025
@finnbear finnbear changed the title feat(server): start h1 header read timeout when conn is idle, a de facto idle timeout. feat(server): start h1 header read timeout when conn is idle, a de facto idle timeout Jan 14, 2025
@finnbear finnbear changed the title feat(server): start h1 header read timeout when conn is idle, a de facto idle timeout fix(server): start h1 header read timeout when conn is idle, a de facto idle timeout Jan 16, 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.

hyper::Error::is_timeout returns false on a HeaderTimeout. add conn_idle_timeout for server
1 participant