-
Notifications
You must be signed in to change notification settings - Fork 524
Handle multiple tokens in Connection header (#1170) #1178
Conversation
@@ -234,12 +286,13 @@ protected virtual void OnConsumedBytes(int count) | |||
var connection = headers.HeaderConnection.ToString(); |
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.
Wouldn't it be more efficient to foreach
the HeaderConnection? Instead of string.Joining like we do in ToString()
?
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.
Talked to @CesarBS and he points out that we don't split headers by commas so this will only matter when there are multiple Connection
headers.
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.
Updated - not calling ToString()
anymore.
|
||
while (ch < end) | ||
{ | ||
if ((*ch | 0x20) == 'k' && (end - ch) >= 9) |
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.
Maybe add a comment that the first characters of connection
cannot be whitespace.
{ | ||
if ((*ch | 0x20) == 'k' && (end - ch) >= 9) | ||
{ | ||
if ((*++ch | 0x20) == 'e' && |
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.
Could we cast to a ulongs and uints to compare 4 or 2 characters at a time? Can we use Benchmark.NET to determine comparing larger types is faster than the the character-by-character version, and that pointers in general are faster than foreach
ing the string.
When I wrote similar code for ValidateHeaderCharacters
I found foreach was faster than fixing the string and comparing using char pointers. You can see the numbers I got here. The foreach version was TestValidateHeaderValues1
and the char*
version was TestValidateHeaderValues2
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.
Also for (var i = 0...
string[i] ==
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.
For ulong compare you want to use the magic number ;-) Gowan....
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.
for (var i = 0... string[i] ==
is TestValidateHeaderValues4
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.
The magic number for Vector and long in the linked Channel repo are a bit down on what they could be as they Slice on each step.
45173dd
to
763e925
Compare
{ | ||
return new ForRemainingData(context); | ||
return new ForRemainingData(upgrade, context); |
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.
Did you decide what to do yet about making sure the Request.Body is not readable on Upgrade requests?
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.
Why should we go out of our way to prevent that?
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.
Poorly coded request buffering middleware?
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.
The request has no body by any given metric, so anything that tried to read from the body would hang when it should just return 0.
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.
Let's track this as a separate issue.
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 approve, but am not well-versed in all the performance implications. The change looks correct though, which is my primary concern.
ch++; | ||
isKeepAlive = ch == end || *ch == ',' || *ch == ' '; |
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.
Is finding a space sufficient? Wouldn't you also need to ensure the space(s) are followed by a ',' or the end of string?
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.
The space or comma char delimits the token, that's all I care about here. The following spaces/commas will be skipped later.
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 about this test case? "keep-alive jk!"
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.
Damn 😆
I think we should do the same thing when we check whether keep-alive is set on the response Connection header in Frame.CreateResponseHeader. I just created this issue to better handle the Transfer-Encoding request header. |
Updated to parse Some benchmarks (
|
Another benchmark, now comparing against using
Here's the code for those:
|
[Flags] | ||
public enum ConnectionOptions | ||
{ | ||
None = 0, |
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.
Why have this instead of returning a nullable ConnectionOptions
?
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.
Flags enums should always have a None
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.
Ah, just read through https://msdn.microsoft.com/en-us/library/ms182149.aspx 👍
@CesarBS Out of curiosity, did you include the call to |
I thought we banned the use of HasFlag, it's pretty awful (it boxes the enum and it does a type check wtf 😄 ) /cc @rynowak |
Thanks guys, I wasn't aware of that. Benchmarks:
|
Updated. |
f50a20a
to
1ffad5c
Compare
#1170
@halter73 @anurse