-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
WinHttpHandler: Read HTTP/2 trailing headers #46602
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,14 @@ internal sealed class WinHttpResponseStream : Stream | |
{ | ||
private volatile bool _disposed; | ||
private readonly WinHttpRequestState _state; | ||
private readonly HttpResponseMessage _responseMessage; | ||
private SafeWinHttpHandle _requestHandle; | ||
private bool _readTrailingHeaders; | ||
|
||
internal WinHttpResponseStream(SafeWinHttpHandle requestHandle, WinHttpRequestState state) | ||
internal WinHttpResponseStream(SafeWinHttpHandle requestHandle, WinHttpRequestState state, HttpResponseMessage responseMessage) | ||
{ | ||
_state = state; | ||
_responseMessage = responseMessage; | ||
_requestHandle = requestHandle; | ||
} | ||
|
||
|
@@ -126,6 +129,7 @@ private async Task CopyToAsyncCore(Stream destination, byte[] buffer, Cancellati | |
int bytesAvailable = await _state.LifecycleAwaitable; | ||
if (bytesAvailable == 0) | ||
{ | ||
ReadResponseTrailers(); | ||
break; | ||
} | ||
Debug.Assert(bytesAvailable > 0); | ||
|
@@ -142,12 +146,17 @@ private async Task CopyToAsyncCore(Stream destination, byte[] buffer, Cancellati | |
int bytesRead = await _state.LifecycleAwaitable; | ||
if (bytesRead == 0) | ||
{ | ||
ReadResponseTrailers(); | ||
break; | ||
} | ||
Debug.Assert(bytesRead > 0); | ||
|
||
// Write that data out to the output stream | ||
#if NETSTANDARD2_1 | ||
await destination.WriteAsync(buffer.AsMemory(0, bytesRead), cancellationToken).ConfigureAwait(false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any observable advantages from using this overload of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is an error when not calling it on netstandard2.1:
|
||
#else | ||
await destination.WriteAsync(buffer, 0, bytesRead, cancellationToken).ConfigureAwait(false); | ||
#endif | ||
} | ||
} | ||
finally | ||
|
@@ -240,7 +249,14 @@ private async Task<int> ReadAsyncCore(byte[] buffer, int offset, int count, Canc | |
} | ||
} | ||
|
||
return await _state.LifecycleAwaitable; | ||
int bytesRead = await _state.LifecycleAwaitable; | ||
|
||
if (bytesRead == 0) | ||
{ | ||
ReadResponseTrailers(); | ||
} | ||
|
||
return bytesRead; | ||
} | ||
finally | ||
{ | ||
|
@@ -249,6 +265,37 @@ private async Task<int> ReadAsyncCore(byte[] buffer, int offset, int count, Canc | |
} | ||
} | ||
|
||
private void ReadResponseTrailers() | ||
{ | ||
// Only load response trailers if: | ||
// 1. HTTP/2 or later | ||
// 2. Response trailers not already loaded | ||
if (_readTrailingHeaders || _responseMessage.Version < WinHttpHandler.HttpVersion20) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OS version check here? |
||
{ | ||
return; | ||
} | ||
|
||
_readTrailingHeaders = true; | ||
|
||
var bufferLength = WinHttpResponseParser.GetResponseHeaderCharBufferLength( | ||
_requestHandle, | ||
Interop.WinHttp.WINHTTP_QUERY_RAW_HEADERS_CRLF | Interop.WinHttp.WINHTTP_QUERY_FLAG_TRAILERS, | ||
isTrailingHeaders: true); | ||
|
||
if (bufferLength != 0) | ||
{ | ||
char[] trailersBuffer = ArrayPool<char>.Shared.Rent(bufferLength); | ||
try | ||
{ | ||
WinHttpResponseParser.ParseResponseHeaders(_requestHandle, Interop.WinHttp.WINHTTP_QUERY_RAW_HEADERS_CRLF | Interop.WinHttp.WINHTTP_QUERY_FLAG_TRAILERS, _responseMessage, trailersBuffer, stripEncodingHeaders: false, isTrailers: true); | ||
} | ||
finally | ||
{ | ||
ArrayPool<char>.Shared.Return(trailersBuffer); | ||
} | ||
} | ||
} | ||
|
||
public override int Read(byte[] buffer, int offset, int count) | ||
{ | ||
return ReadAsync(buffer, offset, count, CancellationToken.None).GetAwaiter().GetResult(); | ||
|
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
ERROR_WINHTTP_HEADER_NOT_FOUND
here to fail silently whenWINHTTP_QUERY_FLAG_TRAILERS
is unsupported?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.
No, it is to handle HTTP responses that don't have any trailers. A response will always have headers, but trailers are optional.
I think it is better to have an OS version check to see whether trailers are supported. That would determine whether the call is made to begin with.
How are Windows version checks does in this library? (or other runtime libraries)
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.
In this library these checks are unprecedental, setting unsupported options usually leads to
WinHttpException
. I've seen checks onEnvironment.OSVersion
in other BCL libs though .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 if WinHttp decides to backport the feature to earlier OS versions? I would feel more comfortable with established pattern of relying on
WinHttpException
. Is that possible?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.
With the current design, this feature is implicit (filling the collection whenever possible), so throwing is not an option.
However I think that checking the error code after the first
WinHttpQueryHeaders
shall be a sufficient (and actually better) solution.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.
One thing to consider here --- we should be very careful about having an API that, depending on OS version:
a) works as expected.
b) appears to work, but has an empty collection with zero notice to user.
This will be very fragile.
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.
@scalablecory I tried to address this in my proposal in #44778 (comment). I suggest to discuss the API under the issue, comments and ideas are more than welcome there.
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 discussed OS support detection for trailers with WinHttp team and this is their suggestion:
They prefered doing that over hardcoding an OS version number because WinHttp trailer support could be backported to more Windows versions.
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 guess these are distinct features which have been introduced in the same release right? What if they get misaligned because trailing headers get backported to an earlier version?
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 presume so. This is the WinHttp team's suggestion. They recommended against checking OS version.