Handle non "chunked" Transfer-Encoding header values (#1181) #1185
Conversation
|
No impact on plaintext benchmark: dev: 1210317.84 RPS |
|
For compression |
| { | ||
| await connection.SendAll( | ||
| "POST / HTTP/1.1", | ||
| "Transfer-Encoding: not-chunked", |
There was a problem hiding this comment.
Can we add a test where the request includes both a Content-Length and a non-chunked Transfer-Encoding?
| @@ -812,6 +812,8 @@ private Task WriteSuffix() | |||
| var responseHeaders = FrameResponseHeaders; | |||
| var hasConnection = responseHeaders.HasConnection; | |||
| var connectionOptions = hasConnection ? FrameHeaders.ParseConnection(responseHeaders.HeaderConnection) : ConnectionOptions.None; | |||
| var hasTransferEncoding = responseHeaders.HasTransferEncoding; | |||
| var transferCoding = hasTransferEncoding ? FrameHeaders.GetFinalTransferCoding(responseHeaders.HeaderTransferEncoding) : TransferCoding.None; | |||
There was a problem hiding this comment.
Nit: HasConnection and HasContentLength both simply call StringValues.Count. It would be simpler to have ParseConnection and GetFinalTransferCoding check the Count themselves and return None rather than doing the Has checks here.
There was a problem hiding this comment.
Just realized I don't even have to check Count - the foreach simply won't iterate over anything and those methods will return None.
|
|
||
| if (hasConnection) | ||
| { | ||
| responseHeaders.SetRawConnection("close", _bytesConnectionClose); |
There was a problem hiding this comment.
I like setting _keepAlive to false, but I'm not sure about changing headers that the user as already explicitly set even to save them from violating the HTTP spec. Do we do that for other headers?
|
Updated. |
cesarblum
force-pushed
the
cesarbs/transfer-encoding-proper-handling
branch
from
be7ab02 to
7763bf3
Compare
|
Ping. |
|
|
cesarblum
force-pushed
the
cesarbs/transfer-encoding-proper-handling
branch
from
7763bf3 to
2940895
Compare
#1181
chunkedis not final token inTransfer-Encodingchunkedis not final token in responseTransfer-EncodingPoint for discussion: should we insert
chunkedand auto-chunk responses that don't havechunkedinTransfer-Encoding? Or should we leave it up to the app to chunk the response in those cases? The problem with not auto-chunking is that Kestrel will now close the connection if such a response is sent.@halter73 @davidfowl @Tratcher @mikeharder