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

Server idle/read/write timeout support #1628

Open
sfackler opened this issue Aug 9, 2018 · 6 comments
Open

Server idle/read/write timeout support #1628

sfackler opened this issue Aug 9, 2018 · 6 comments
Labels
A-http2 Area: HTTP/2 specific. A-server Area: server. B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature.

Comments

@sfackler
Copy link
Contributor

sfackler commented Aug 9, 2018

On the server side, it's important to be able to clear out connections that have been idle for too long or are somehow broken. For HTTP/1, this can be done pretty simply by setting stream-level read and write timeouts. However, things aren't so simple for HTTP/2 since it's multiplexed. In particular, the server is always attempting to read from an HTTP/2 socket, so if a single request is active that takes more than the read timeout to process, the connection gets killed.

To handle this properly, I think we'll need hyper-level timeout configuration. In particular, I care about being able to shut down connections that have been idle for some period of time, and detect when reading from a client or writing to it is taking too long.

There are also some potential higher level timeouts to defend against e.g. slow loris attacks by placing a cap on the time a client is allowed to send the complete set of headers.

@seanmonstar
Copy link
Member

For an idle timeout in HTTP2, having the AsyncRead register a timeout for something longer than a request should ever take could work, right? Like, if the timeout is 5 minutes (or 10 or something), that should be long enough to not interrupt requests?

@seanmonstar seanmonstar added A-server Area: server. C-feature Category: feature. This is adding a new feature. B-rfc Blocked: More comments would be useful in determine next steps. A-http2 Area: HTTP/2 specific. labels Aug 9, 2018
@sfackler
Copy link
Contributor Author

sfackler commented Aug 9, 2018

Yeah, that's what we're doing right now (30 second timeouts for HTTP/1 and 10 minute timeouts for HTTP/2), but ideally we'd be able to handle this more precisely.

Thinking about it a bit more, the minimum set of functionality that would need to be in hyper is just an idle timeout and a request header timeout. Read/write timeouts can be managed by the service implementation outside of hyper.

@seanmonstar
Copy link
Member

That does seem reasonable. So far, punting on timeout support has had the benefit of not being tied to a specific timer (or runtime). The core of hyper still works if you disable the runtime feature, but I think using tokio-timer for these timeouts internally would change that.

@sfackler
Copy link
Contributor Author

sfackler commented Aug 9, 2018

I think we can preserve those goals. Initially, timeouts could require the runtime feature, and we could add an abstraction over timers to support other runtimes.

@sfackler
Copy link
Contributor Author

I'd like to start moving on this. I think the simplest first step would be to handle server-side idle timeouts.

As an initial approach, I'm thinking of tying support to the runtime feature - the setters like Http::set_idle_timeout etc will be cfg'd to that. To keep the internals from becoming a huge nightmare, we can make a stub no-op Delay type for cfg(not(runtime)) so the internal implementation can just assume it has timers.

If there's interest we could make a trait abstraction for timers to support non-tokio runtimes, but that doesn't seem necessary initially.

On the HTTP/2 side, we can identify if we're idle with some reference counting scheme with the H2Streams, but it might be better long term to expose this information directly from h2.

Seem reasonable?

@sfackler
Copy link
Contributor Author

sfackler commented Apr 21, 2019

Hmm, the HTTP/2 approach won't really work the way you might want since it'd ignore pings...

EDIT: Nevermind, Go's behavior is to ignore pings for the purpose of idling, which makes sense to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-http2 Area: HTTP/2 specific. A-server Area: server. B-rfc Blocked: More comments would be useful in determine next steps. C-feature Category: feature. This is adding a new feature.
Projects
None yet
Development

No branches or pull requests

2 participants