-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Add gRPC standard headers to known headers #24714
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
Conversation
| { | ||
| "ETag", | ||
| "Grpc-Message", | ||
| "Grpc-Status" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about splitting HttpRequestHeaders into Http1RequestHeaders and Http2RequestHeaders? If we include the pseudo-header fields, there are now 9 request header fields in HttpRequestHeaders that I wouldn't expect to be used with HTTP/1.x (though maybe grpc-web means the gRPC headers are also common over HTTP/1.x now).
I'm concerned that using the same HttpRequestHeaders class for HTTP/1.x and HTTP/2 and adding too many HTTP/2-specific headers will slow down HTTP/1.x unnecessarily. We already have 4 HTTP/2 pseudo-header fields in requestPrimaryHeaders. TryGetValueFast will check if the requested header name matches these primary headers before other header names of the same length. Any extra header fields waste memory and slow down methods like Clear, TryGetValueFast, Append, etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that would be a good improvement. I don't know if there will be time for .NET 5.
On primary headers: the HTTP/2 pseudo-headers can probably be removed from primary headers. Pseudo-headers are sent in HEADERS frame as static table indexes. They'll use the upcoming optimizations here: #24730
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these exact header names used for both HTTP/2 gRPC and gRPC-Web? If so, I guess these warrant being in a possible Http1RequestHeaders dictionary anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gRPC-Web headers are the same, and gRPC-Web is usable in HTTP/1.1 and 2. So yes it is useful for them to be in a potential Http1RequestHeaders.
The only HTTP/2 specific headers are the pseudo ones.
|
Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:
|
davidfowl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Fixes #24701
Before:
After:
There are also small performance improvements. gRPC headers are get and set in header collections using fast checks. They don't fallback to using the unknown dictionary collection.