WinHttp: always read HTTP/2 streams to the end

[antonfirsov]

By it's default behavior, WinHttp stops reading the stream when Content-Length is specified, this prevents us to read the remaining trailers. By opting in into WINHTTP_OPTION_REQUIRE_STREAM_END, WinHttpHandler behavior becomes equivalent of SocketsHttpHandler. Note that this means if the actual data is longer than Content-Length, we will read all the extra data bytes in both handlers, which we probably shouldn't do, but that's a topic we should investigate separately.

I don't fully understand the impact of applying WINHTTP_OPTION_REQUIRE_STREAM_END in HTTP/1.1, so I'm applying it only for HTTP 2.0 requests.

The PR also adds equivalent Content-Length tests for SocketsHttpHandler for consistency.

Fixes #60291

/cc @JamesNK

[ghost]

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

---

In it's default behavior, WinHttp stops reading the stream when Content-Length is specified, this prevents us to read the remaining trailers. By opting in into WINHTTP_OPTION_REQUIRE_STREAM_END, WinHttpHandler behavior becomes equivalent of SocketsHttpHandler. Note that this means if the actual data is longer than Content-Length, we will read all the extra data bytes in both handlers, which we probably shouldn't do, but that's a topic we should investigate separately.

I don't fully understand the impact of applying WINHTTP_OPTION_REQUIRE_STREAM_END in HTTP/1.1, so I'm applying it only for HTTP 2.0 requests.

The PR also adds equivalent Content-Length tests for SocketsHttpHandler for consistency.

Fixes #60291

Author:	antonfirsov
Assignees:	-
Labels:	area-System.Net.Http
Milestone:	-

[JamesNK]

Stream end is a http2 and http3 concept. I assume it has no impact on http11. Probably worth double checking with winhttp folks.

I would like a unit test of a http2 server returning more content than its content-length defines. We can check what socketshttphandler does, what winhttphandler does today (pre-pr) and what it does after adding this flag.

My experience is in the server side of http, but there we validate the content length and error if doesn’t match the actual content.

Correct me if I’m wrong @Tratcher :)

[Tratcher]

The HTTP/2 spec says having a Content-Length mismatch the total data frames is a stream level protocol violation and should result in RST. It's worth confirming WinHttp enforces that.

[antonfirsov]

Since this PR is a candidate for our backport process I'd like to stick to the minimal changes necessary to fix #60291. Opened #62906 to track our lack of response Content-Length enforcement, current behavior is described in that issue.

[JamesNK]

Ok! As long as the impact of the setting is understood then I’m fine with looking at content-length separately.

[antonfirsov]

@JamesNK I pushed b4b70f0 to set the option right on the session handle, since the WinHttp team confirmed it doesn't have effect on Http1.1.

If you are fine with the current state, I'm happy to merge this.

[antonfirsov]

/azp run runtime-libraries-coreclr outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[JamesNK]

[antonfirsov]

Outerloop failures are unrelated: #63008 #63009

[stephentoub]

NetEventSource.Info(this, $"Failed to enable WINHTTP_OPTION_REQUIRE_STREAM_END error code: {Marshal.GetLastWin32Error()}");

[antonfirsov]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1654653739