Skip to content

HTTP/3 RequestHeadersTimeout #30638

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

Merged
merged 1 commit into from
Apr 23, 2021
Merged

HTTP/3 RequestHeadersTimeout #30638

merged 1 commit into from
Apr 23, 2021

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented Mar 4, 2021

Adds RequestHeadersTimeout support to HTTP/3

Addresses #29702

(this PR is still rough. Creating it early to ask questions)

The HTTP/3 approach doesn't use ITimeoutControl for this feature because multiple time controls would be required per connection, and checking them in Tick() would require locking the streams dictionary, foreaching over the values, and evaluating each timeout control.

The approach used instead is like HTTP/2's drain stream queue. New streams are added to a concurrent queue, and each tick streams are checked to see whether they have received headers and are in a "started" state.

I made RequestHeadersTimeout apply to both request streams and control streams. They each have the same attack vector: creating streams from the client and then never using them. Control streams use the same timeout to receive the control stream header.

A future possible optimization is to check whether the first read of an incoming stream completes immediately, and the first HEADERS frame or control stream frame header is immediately available. If it is then the stream can skip being added to the startingStreamsQueue.


V3: On connection tick, the stream dictionary is locked and iterated over to check for streams (both request streams and inbound control streams) to see if they have received a header.

I chose not to use TimeoutControl here to avoid each stream requiring its own TimeoutControl for this one thing AND the connection having a timeout control which streams also use to track data rates.

This PR starts to unify how inbound control streams and request streams are acted on. They are both put into the streams dictionary so the same logic is applied to streams from the client:

  • Checking a stream has started
  • Aborting streams when connection aborts

@halter73 and I discussed the foreach in the lock, and while it isn't ideal:

  • It happens once per second
  • Little happens in it
  • Default QUIC stream limit for requests is 100 so it is a bounded collection
  • Compared to other locks, it will have much less contention. For example, the HTTP/2 level connection locks its framewriter multiple times per request and does much more complex work inside the lock.

@ghost ghost added the area-servers label Mar 4, 2021
@JamesNK JamesNK force-pushed the jamesnk/http3-ratetimeouts branch from 6f7055c to 7a39bf3 Compare March 22, 2021 03:59
@JamesNK JamesNK force-pushed the jamesnk/http3-ratetimeouts branch from 7a39bf3 to 6fa0432 Compare April 19, 2021 00:59
@JamesNK JamesNK force-pushed the jamesnk/http3-ratetimeouts branch from b52eac2 to 74956b3 Compare April 19, 2021 23:56
@JamesNK JamesNK force-pushed the jamesnk/http3-ratetimeouts branch from dba2133 to 32cdb30 Compare April 22, 2021 11:07
@JamesNK JamesNK force-pushed the jamesnk/http3-ratetimeouts branch from 152e0d2 to e35b224 Compare April 23, 2021 02:48
@JamesNK JamesNK merged commit 202c1e8 into main Apr 23, 2021
@JamesNK JamesNK deleted the jamesnk/http3-ratetimeouts branch April 23, 2021 05:16
@ghost ghost added this to the 6.0-preview5 milestone Apr 23, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Dec 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants