Implement Kestrel Request PipeReader

[jkotalik]

For #4757 and #4697.

This PR is the second half of exposing pipes in Kestrel. It does a few main things:

• Makes Content-Length requests directly read from the connection pipe rather than from a request pipe
• Moves RequestPipe logic (PumpAsync) into the Chunked message body.
• Changes Streams => BodyControl and makes all body control calls be done through the HttpRequestPipeReader/HttpResponsePipeWriter

I would consider this PR semi-WIP. I think the overall design is solid, but there is some code duplication I can remove. I also want to add some more tests for Http2 message bodies and tests for ConsumeAsync. Finally, I think I'll need to add some benchmarks for Content-Length request reading and maybe add a few benchmark/inmemory scenarios too. Also, after scanning this PR, there is a lotttt of commented out code 🐋 .

cc/ @benaadams @shirhatti @JamesNK as I know you guys care 😄

[jkotalik]

For some reason I didn't have the option to make a draft PR 😢

[jkotalik]

Some commentary to guide feedback.

[jkotalik]

This class has very little changed from the original implementation. It now contains the RequestBodyPipe rather than MessageBody and continues to call PumpAsync().

[halter73]

I don't think I understand this. Shouldn't TryRead check IsCanceled and throw a BadHttpRequestException too?

[davidfowl]

Try* API shouldn’t throw. Makes them unusable, so we should avoid it where possible

[jkotalik]

Yeah I was trying to avoid throwing in Try methods. However, TryRead throws internally in pipes, so we should consider fixing that in pipes.

[halter73]

Try* API shouldn’t throw. Makes them unusable, so we should avoid it where possible

Like @jkotalik points out, TryRead is a little special. Errors also need to be raised from TryRead when the writer completes with an error. TryRead should only return false when the PipeReader is still active but there's no data available to currently read.

[JamesNK]

A little more info about the TryXXX pattern: TryXXX doesn't mean the method should never throw an exception. It means it shouldn't throw an exception for a certain use-case.

For example, int.TryParse will return false if the string cannot be parsed to an integer. But it will throw an ArgumentException if you give it an invalid NumberStyles value.

[davidfowl]

TryRead should only return false when the PipeReader is still active but there's no data available to currently read.

Sure, it has to throw sometimes in those rare cases where the input is invalid. I don't know that I agree with the above.

[halter73]

File an issue for this.

[jkotalik]

I will once I merge this PR.

[halter73]

On the right track. I want so see better code sharing before I approve this.

[jkotalik]

Updated. I think I saw one test fail with timing (think there is a different exception message), I’ll take a look at it later today/tomorrow.

[benaadams]

Conflicting files
`src/Servers/Kestrel/test/InMemory.FunctionalTests/RequestTests.cs`

[JamesNK]

Yes, but I will verify now by running it against gRPC.

Thanks! It's important to test the 3 clients under examples/clients run without issue. The functional tests use TestServer which I'm guessing will still use the stream adapters.

[davidfowl]

@Eilon can we make this PR contingent on no gRPC regressions? I would be good to merge this today.

[Eilon]

@davidfowl contingent not just on some one-off gRPC testing, but on us asserting that the risk is low for all cases (preview 3 works just fine now, I don't want to break anything because we rushed).

[jkotalik]

@Eilon tested against all HTTP2 clients in gRPC. This is a low risk change for gRPC because it barely changed anything with HTTP2 . See: https://github.com/aspnet/AspNetCore/pull/7603/files#diff-82d8f6d18b1c89f77b834073ac706d54R4 for the changes to HTTP2 . This effectively just exposes the underlying HTTP2 stream pipe.

[Eilon]

Approved for preview 3.

[jkotalik]

All required checks passed.