Skip to content

fix(relay): bound HTTP request reads - #281

Merged
andrei-21 merged 5 commits into
mainfrom
fix/http-timeout
Aug 24, 2026
Merged

fix(relay): bound HTTP request reads#281
andrei-21 merged 5 commits into
mainfrom
fix/http-timeout

Conversation

@andrei-21

Copy link
Copy Markdown
Contributor

Enable Hyper's HTTP/1 header timeout and enforce a hard deadline
on request bodies so stalled clients cannot retain file descriptors.

Restrict the server to HTTP/1 because protocol auto-detection occurs
before Hyper's header timeout.

@andrei-21
andrei-21 requested a review from SeverinAlexB July 24, 2026 17:42
@SeverinAlexB

Copy link
Copy Markdown
Contributor

Downgrading to HTTP/1 only is quite bad. Did you research if there are not alternatives?

@andrei-21

Copy link
Copy Markdown
Contributor Author

Downgrading to HTTP/1 only is quite bad. Did you research if there are not alternatives?

Alternatives are:

  • support HTTP/2 in relay and properly defend from the issue, but it requires a custom acceptor, see the commit
  • support only HTTP/1 but enable HTTP/2 on nginx and forward it as HTTP/1
  • move from axum to Salvo, but it is a totally different framework

@andrei-21
andrei-21 requested a review from 86667 August 10, 2026 13:47
@86667

86667 commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

This is a lot of complex code just for timeouts.

Whats wrong with a simple hard deadline per-connection using existing exposed config options in hyper? Do we expect for there to be requests which take more than 30s to return? If so, could we just set the deadline to 60s instead, surely that is high enough for all requests which are going to be returned to return. Especially considering the payloads are tiny.

I need convincing here that the complexity justifies the gain of allowing users of pkarr to indefinitely wait for dht responses.

@andrei-21

Copy link
Copy Markdown
Contributor Author

Whats wrong with a simple hard deadline per-connection using existing exposed config options in hyper?

Nothing inherently. A hard per-connection deadline is simpler, but Hyper has no direct option for it.

Its downside is that expiry kills valid in-flight requests and all multiplexed HTTP/2 streams. A sufficiently long deadline may be acceptable, but it differs from the current logic, which times out only while waiting for a request and lets active requests finish.

@andrei-21

Copy link
Copy Markdown
Contributor Author

@86667 cb0acc7 commit implements a smarter version of your idea.
The commit implements a fixed maximum connection lifetime. Once reached, the server gracefully stops accepting new requests, allows in-flight requests up to 30 seconds to finish, then forcibly closes the connection. It also adds initial-header timeouts, connection limits, and HTTP/1–HTTP/2 resource bounds.

Comment thread relay/src/http_server.rs Outdated
Comment thread relay/src/http_server.rs Outdated
@SeverinAlexB

Copy link
Copy Markdown
Contributor

Aren't we going the wrong direction with this PR? Instead of Hyper managing the timeout, shouldn't we either

  • timeout the pkarr call with tokio::time::timeout.
  • Fix pkarr so it actually always returns within the given set timeout (preferred).

This feels like fighting symptoms instead of addressing the root cause. Other app working with pkarr will have the same issues.

Enable Hyper's HTTP/1 header timeout and enforce a hard deadline
on request bodies so stalled clients cannot retain file descriptors.

Restrict the server to HTTP/1 because protocol auto-detection occurs
before Hyper's header timeout.
Enforce a first-request deadline across protocol detection and the HTTP/2
handshake, then defer to protocol-specific header and body timeouts.
Replace the request-deadline stream wrapper with explicit Hyper
connection handling so stalled clients cannot retain relay resources
indefinitely.

Bound concurrent connections, initial request headers, total connection
age, and graceful draining. Preserve peer address extraction and apply
protocol-specific HTTP/1 and HTTP/2 limits.
@andrei-21

Copy link
Copy Markdown
Contributor Author

Aren't we going the wrong direction with this PR? Instead of Hyper managing the timeout, shouldn't we either

* timeout the pkarr call with `tokio::time::timeout`.

* Fix pkarr so it actually always returns within the given set timeout (preferred).

This feels like fighting symptoms instead of addressing the root cause. Other app working with pkarr will have the same issues.

The failure reproduced here happens before pkarr is called. Opening 200 TCP connections and sending no HTTP data retained 200 file descriptors indefinitely. The request never reaches Axum, the relay handler, or the DHT client, so a timeout around the pkarr call cannot address it.
This PR therefore gives the HTTP transport its own resource bounds: incomplete-header deadlines, request-body deadlines, connection/stream limits, and bounded connection lifetime. These are separate from the application-level DHT timeout and remain necessary even if pkarr always honors its timeout.

@andrei-21
andrei-21 requested a review from 86667 August 21, 2026 10:52
@SeverinAlexB

Copy link
Copy Markdown
Contributor

I am confused. Are you saying every hyper http server is bugged and easy to DDOS?

@andrei-21

Copy link
Copy Markdown
Contributor Author

I am confused. Are you saying every hyper http server is bugged and easy to DDOS?

Yes and no. Axum intentionally focuses on HTTP-level routing and handling, while protections such as connection, request, and body timeouts often span multiple layers and must be configured separately.

Comment thread relay/src/http_server.rs
@andrei-21
andrei-21 merged commit 6e23e39 into main Aug 24, 2026
9 checks passed
@andrei-21
andrei-21 deleted the fix/http-timeout branch August 24, 2026 18:01
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.

3 participants