Trim response headers in CurlHandler#4194
Conversation
Apply same header response trimming done by WinHttpHandler. Fixes #4187 (At some point I'd like to go through and replace all Substring(...).Trim() calls with a more efficient SubstringTrim(...) that doesn't allocate an intermediate string. But in the meantime, this addresses the problem.)
Yay! Was just going to recommend this change. |
There was a problem hiding this comment.
Nit:
Trim will remove characters which return true for Char.IsWhitespace. Right?
The spec says we can have what it calls OWS here (optional whitespace). And OWS is defined as SPACE | HTAB.
Would it be possible to use Trim(' ', '\t') here? Otherwise, theoretically, CurlHandler could end up having different values for headers.
Or, if needed I can handle it in #4131
There was a problem hiding this comment.
I'm following what's being done in WinHttpHandler:
https://github.com/dotnet/corefx/blob/master/src/System.Net.Http.WinHttpHandler/src/System/Net/Http/WinHttpResponseParser.cs#L201
@davidsh, is WinHttpHandler too aggressive here, or have I misunderstood?
There was a problem hiding this comment.
@kapilash, if you'd like to incorporate the fix into your change, that's fine, too, and you can close this. Either way.
There was a problem hiding this comment.
If @davidsh says this needs to be fixed, I will handle it in the other PR. As you pointed out, CurlHandler and WinHttpHandler are in sync with Trim().
On a side note, may be, we should consider moving the header parsing to a common place.
There was a problem hiding this comment.
is WinHttpHandler too aggressive here, or have I misunderstood?
To be more efficient, it might have been better for WinHttpHandler to specify the specific list of "whitespace" characters to trim away based on what the HTTP protocol would allow as "ignorable whitespace". Instead, I simply used Trim(). In practice, for example, there can't be things like CR or LF characters in the header values because it would never parse as valid HTTP protocol in the first place.
I think the safest thing to do right now is to keep CurlHandler the same as WinHttpHandler and use Trim(). In parallel, I will consult with other HTTP experts here to determine what a better, more mimimal set of trim characters could be. Then, later, we can optimize the algorithm and perhaps re-factor into some Common code modules that both handlers could share.
|
LGTM! |
|
@stephentoub Also, this parsing bug needs to get into RC1. Can you drive this with shiproom? |
|
LGTM. Thanks for fixing, @stephentoub |
Trim response headers in CurlHandler
…header Trim response headers in CurlHandler Commit migrated from dotnet/corefx@e45d704
7 participants
Apply same header response trimming done by WinHttpHandler.
Fixes #4187
cc: @kapilash, @davidsh
(At some point I'd like to go through and replace all Substring(...).Trim() calls with a more efficient SubstringTrim(...) that doesn't allocate an intermediate string. But in the meantime, this addresses the problem.)