Skip to content

Support SETTINGS_MAX_HEADER_LIST_SIZE on HTTP/2 and HTTP/3 #78193

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

Closed
MihaZupan opened this issue Nov 10, 2022 · 4 comments · Fixed by #79281
Closed

Support SETTINGS_MAX_HEADER_LIST_SIZE on HTTP/2 and HTTP/3 #78193

MihaZupan opened this issue Nov 10, 2022 · 4 comments · Fixed by #79281
Assignees
Labels
area-System.Net.Http bug tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Milestone

Comments

@MihaZupan
Copy link
Member

HTTP/2 and HTTP/3 specs define an advisory SETTINGS_MAX_HEADER_LIST_SIZE setting:

This advisory setting informs a peer of the maximum field section size that the sender is prepared to accept, in units of octets. The value is based on the uncompressed size of field lines, including the length of the name and value in units of octets plus an overhead of 32 octets for each field line.

An endpoint can use the SETTINGS_MAX_HEADER_LIST_SIZE to advise peers of limits that might apply on the size of uncompressed field blocks. This setting is only advisory, so endpoints MAY choose to send field blocks that exceed this limit and risk the request or response being treated as malformed

So while HttpClient isn't violating the spec by not enforcing this limit, we could play nicer here.

The fact that we're not enforcing the limit leads to situations that can be very difficult to debug. The server is free to close the connection in situations where it receives too many header bytes. On HttpClient's side, we're then forced to fail all the requests that are being multiplexed over that connection and report a non-helpful exception, while logging on the server may not even indicate that any such offending request was received.

System.Net.Http.HttpRequestException: An error occurred while sending the request.
System.IO.IOException: The request was aborted.
System.IO.IOException: The response ended prematurely while waiting for the next frame from the server.
    at System.Net.Http.Http2Connection.<ReadFrameAsync>g__ThrowMissingFrame|57_1()   
    at System.Net.Http.Http2Connection.ReadFrameAsync(Boolean initialFrame)  
    at System.Net.Http.Http2Connection.ProcessIncomingFramesAsync()   
    --- End of inner exception stack trace ---
    at System.Net.Http.Http2Connection.ThrowRequestAborted(Exception innerException)
    at System.Net.Http.Http2Connection.Http2Stream.CheckResponseBodyState() 
    at System.Net.Http.Http2Connection.Http2Stream.TryEnsureHeaders()
    at System.Net.Http.Http2Connection.Http2Stream.ReadResponseHeadersAsync(CancellationToken cancellationToken)

I believe that HttpClient should enforce this setting if the server announced it. We should do so by failing the request before we send any data from that request on the shared connection.

@MihaZupan MihaZupan added this to the 8.0.0 milestone Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

HTTP/2 and HTTP/3 specs define an advisory SETTINGS_MAX_HEADER_LIST_SIZE setting:

This advisory setting informs a peer of the maximum field section size that the sender is prepared to accept, in units of octets. The value is based on the uncompressed size of field lines, including the length of the name and value in units of octets plus an overhead of 32 octets for each field line.

An endpoint can use the SETTINGS_MAX_HEADER_LIST_SIZE to advise peers of limits that might apply on the size of uncompressed field blocks. This setting is only advisory, so endpoints MAY choose to send field blocks that exceed this limit and risk the request or response being treated as malformed

So while HttpClient isn't violating the spec by not enforcing this limit, we could play nicer here.

The fact that we're not enforcing the limit leads to situations that can be very difficult to debug. The server is free to close the connection in situations where it receives too many header bytes. On HttpClient's side, we're then forced to fail all the requests that are being multiplexed over that connection and report a non-helpful exception, while logging on the server may not even indicate that any such offending request was received.

System.Net.Http.HttpRequestException: An error occurred while sending the request.
System.IO.IOException: The request was aborted.
System.IO.IOException: The response ended prematurely while waiting for the next frame from the server.
    at System.Net.Http.Http2Connection.<ReadFrameAsync>g__ThrowMissingFrame|57_1()   
    at System.Net.Http.Http2Connection.ReadFrameAsync(Boolean initialFrame)  
    at System.Net.Http.Http2Connection.ProcessIncomingFramesAsync()   
    --- End of inner exception stack trace ---
    at System.Net.Http.Http2Connection.ThrowRequestAborted(Exception innerException)
    at System.Net.Http.Http2Connection.Http2Stream.CheckResponseBodyState() 
    at System.Net.Http.Http2Connection.Http2Stream.TryEnsureHeaders()
    at System.Net.Http.Http2Connection.Http2Stream.ReadResponseHeadersAsync(CancellationToken cancellationToken)

I believe that HttpClient should enforce this setting if the server announced it. We should do so by failing the request before we send any data from that request on the shared connection.

Author: MihaZupan
Assignees: -
Labels:

area-System.Net.Http

Milestone: 8.0.0

@MihaZupan
Copy link
Member Author

Triage: This is a problem that is difficult to diagnose and trace back to the headers, and we've already seen users hitting it (e.g. dotnet/yarp#1875). Moving to 8.0.

@karelz karelz added the bug label Nov 29, 2022
@MihaZupan MihaZupan self-assigned this Nov 29, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 6, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 27, 2022
@MihaZupan
Copy link
Member Author

Reopening for 6.0 and 7.0 servicing

@MihaZupan MihaZupan reopened this Dec 27, 2022
@MihaZupan MihaZupan added the tenet-reliability Reliability/stability related issue (stress, load problems, etc.) label Jan 3, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 4, 2023
@karelz karelz modified the milestones: 8.0.0, 6.0.x Jan 4, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 5, 2023
@karelz
Copy link
Member

karelz commented Jan 6, 2023

Fixed in main (8.0) in PR #79281 and in 7.0.3 in PR #79994 and in 6.0.14 in PR #79997 (for HTTP/2 only).

@karelz karelz closed this as completed Jan 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http bug tenet-reliability Reliability/stability related issue (stress, load problems, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants