Change how HTTP/2 frames are parsed and generated #2893
Conversation
|
@geoffkizer FYI, are you using the Http2Frame type at all? |
|
Not currently, but thanks for the heads up |
|
I had @sebastienros run this in the lab and he saw no performance change. That's not unexpected since this should only improve per connection memory and the standard perf tests focus on stream density. Not regressing mainstream perf is good enough here. |
Tratcher
force-pushed
the
tratcher/frame
branch
from
f0e0c8f
to
cdaf999
Compare
|
Rebased and updated |
| { | ||
| length = 0; | ||
|
|
||
| do | ||
| { | ||
| if (!EncodeHeader(_enumerator.Current.Key, _enumerator.Current.Value, buffer.Slice(length), out var headerLength)) | ||
| { | ||
| if (length == 0 && throwIfNoneEncoded) | ||
| { | ||
| throw new ArgumentOutOfRangeException("The given buffer was too small to encode any headers."); |
There was a problem hiding this comment.
Will this be raised to app code on the first write to the body if an encoded header is greater than the max frame size specified to the client or the server? If this can be induced simply by the client setting a small enough max frame size, I'm not sure that an ArgumentOutOfRangeException is appropriate particularly if there's no way for the app to discover what the client configured the max frame size to be.
If this is typically thrown on the first write to the body, how is this handled and logged if there is no body?
Any particular reason for not using a resource string here?
There was a problem hiding this comment.
Yes it does surface to the application on first write. I added it because I hung multiple tests while refactoring the max frame size handling. Without it this code spins in a loop.
I'm not as concerned about the application running into this. The client isn't allowed to reduce the max frame size bellow 16kb, and if your individual headers are over 16kb then you have other issues. I will change the type to InvalidOperationException.
There was a problem hiding this comment.
How is it logged if the response doesn't contain a body?
There was a problem hiding this comment.
It's logged as a general request error. I've added a little more error handling to abort the connection since the header compression table would now be considered corrupt. There's not a good way to run the normal Http2Connection cleanup code from here. I'm not concerned with making this code path more graceful at the moment, we'll see if users really run into this scenario.
There was a problem hiding this comment.
Filed https://github.com/aspnet/KestrelHttpServer/issues/2926 to address this by splitting header values acrross frames.
Tratcher
force-pushed
the
tratcher/frame
branch
from
71f2a8a
to
d5a5eba
Compare
|
Rebased and updated. |
| { | ||
| length = 0; | ||
|
|
||
| do | ||
| { | ||
| if (!EncodeHeader(_enumerator.Current.Key, _enumerator.Current.Value, buffer.Slice(length), out var headerLength)) | ||
| { | ||
| if (length == 0 && throwIfNoneEncoded) | ||
| { | ||
| throw new HPackEncodingException(CoreStrings.HPackErrorNotEnoughBuffer); |
There was a problem hiding this comment.
I know we don't plan for this to be permanent, but I would still like a test verifying this gets thrown from the first body write.
Tratcher
force-pushed
the
tratcher/frame
branch
from
d5a5eba
to
873c436
Compare
Tratcher
force-pushed
the
tratcher/frame
branch
from
873c436
to
b8423b8
Compare
|
👍 |
#2858 I was able to reduce the request buffer from 16kb to 15 bytes. The response buffer is still needed for compressing headers.
This helped eliminate several data copies for request headers, request and response data, and settings frames.
Converting Http2Frame to a POCO helped eliminate a lot of redundant de/serialization of properties that were accessed multiple times.
Measurements needed.