Skip to content

Rebalance common headers #31495

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 2 commits into from
Apr 28, 2021
Merged

Rebalance common headers #31495

merged 2 commits into from
Apr 28, 2021

Conversation

benaadams
Copy link
Member

@benaadams benaadams commented Apr 3, 2021

Some headers in common are only used for request or response; so move them to those collections rather than having them in both.

Note: this doesn't mean they are common enough to be kept in the collections; however they definitely shouldn't be in both request and response collections.

Moved from both to only Response headers; shrinking Request headers by 72 bytes.

  • Trailer
  • Allow
  • ContentEncoding
  • ContentLanguage
  • ContentLocation
  • ContentMD5
  • ContentRange
  • Expires
  • LastModified

Contributes to #31492

@ghost ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Apr 3, 2021
@Tratcher
Copy link
Member

Tratcher commented Apr 5, 2021

Test failure:
Microsoft.AspNetCore.Server.Kestrel.Core.Tests.Http2HPackEncoderTests.BeginEncodeHeaders_MaxHeaderTableSizeExceeded_EvictionsToFit

Assert.Equal() Failure
↓ (pos 99)
Expected: ···A-32-32-20-47-4D-54-5A-04-67-7A-69-70-C1-1F-28-38-66-6F-6F-3D···
Actual:   ···A-32-32-20-47-4D-54-C0-1F-28-38-66-6F-6F-3D-41-53-44-4A-4B-48···
↑ (pos 99)

@BrennanConroy BrennanConroy added the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Apr 21, 2021
@benaadams
Copy link
Member Author

Test failure:
Microsoft.AspNetCore.Server.Kestrel.Core.Tests.Http2HPackEncoderTests.BeginEncodeHeaders_MaxHeaderTableSizeExceeded_EvictionsToFit

Enumerator ordering; will change to be alpha sorted rather than listing ordered so is less fragile

@benaadams
Copy link
Member Author

Rebased and made the ordering more deteministic so http2 header compression tests are less sensitive to where something is in the list in KnownHeaders.cs

@benaadams
Copy link
Member Author

@BrennanConroy don't think its pending author input anymore?

@benaadams benaadams requested a review from Tratcher April 27, 2021 21:13
@BrennanConroy
Copy link
Member

Yeah, I think the bot is supposed to remove that :/

@BrennanConroy BrennanConroy removed the pr: pending author input For automation. Specifically separate from Needs: Author Feedback label Apr 27, 2021
@Tratcher Tratcher added this to the 6.0-preview5 milestone Apr 28, 2021
@Tratcher Tratcher merged commit 673a551 into dotnet:main Apr 28, 2021
@Tratcher
Copy link
Member

Thanks

@benaadams benaadams deleted the Rebalance-headers branch April 28, 2021 18:11
@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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants