Fix TestServer hang with duplex streaming requests

[JamesNK]

Description

TestServer does not correctly execute bidirectional requests. The HttpClient from a TestServer request will hang reading the response in two situations:

1. The server writes no response content
2. The client stream is not completed.

This PR resolves both of these issues.

Customer Impact

Because of these issues, TestServer can't be used to host gRPC functional tests of bidirectional gRPC calls.

For gRPC functional testing customers will be forced to host an in-process Kestrel instance and call via TCP sockets. This could cause port exhaustion on some OS with a large number of tests.

Regression?

No, TestServer has never correctly supported bidirectional requests.

Risk

Low. TestServer is designed for unit tests, not production use. Note: TestServer ships in Microsoft.AspNetCore.TestHost, an out-of-sdk NuGet package: https://www.nuget.org/packages/Microsoft.AspNetCore.TestHost/

[thefringeninja]

Any chance of getting this into 3.1?

[analogrelay]

Any chance of getting this into 3.1?

Not for 3.1.0, it's locked down tight. It could be considered for a patch to 3.1 though. @JamesNK is there any workaround for this issue?

[Tratcher]

@aspnet-hello triage https://dev.azure.com/dnceng/public/_build/results?buildId=429434

[Tratcher]

Calling Response.Body.FlushAsync or Response.CompleteAsync should mitigate it.

[aspnet-hello]

This comment was made automatically. If there is a problem contact [email protected].

I've triaged the above build.

[JamesNK]

Might be able to have a middleware in the test server instance that does nothing other than flush/complete. That wouldn’t be too intrusive.

Otherwise this could be a 3.1 patch.

[JamesNK]

Calling Response.Body.FlushAsync or Response.CompleteAsync should mitigate it.

I've tested and this isn't enough. The response will be returned to the HttpClient, but the response stream won't close until the request stream is complete. That is because clean up will wait for the request to be completed before the response:

await CompleteRequestAsync();
await CompleteResponseAsync();

I think we'll need to do this as a 3.1 patch.

[JamesNK]

@anurse Added servicing label. Also update description with required template.

[analogrelay]

Do you want this to go in to 3.1 directly or backport from master? The PR is targeting master but you put servicing labels on it :).

If you want it to go to 3.1, retarget to release/3.1 and put the label back on :). I'll take it to tactics.

[JamesNK]

I'd like to put it directly into 3.1 and then have it merged back to master. There is no urgency to get it into 5.0.

I'll retarget.

[analogrelay]

I want to have a chat with you offline about this one @JamesNK .

[analogrelay]

@Tratcher helped me clear this up.

• In 3.0, bidirectional requests worked
• We made changes in 3.1 that regressed this behavior
• This PR is to fix those changes.

Given that, this definitely is worth submitting for patch. Reapplied the labels.

[JamesNK]

No.

• 3.0, bidirectional requests hung.
• 3.1 fixed bidirectional requests hanging. However it was discovered from users that the two scenarios listed here (request does not finish, no content written) still hang.
• This PR fixes the two scenarios.

[mkArtakMSFT]

These were approved for 3.1.2, so updating the milestone accordingly /cc @jamshedd

[vivmishra]

Moved to Mar as per Tactics. Will need to be explicitly approved for Feb, if required.

[analogrelay]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).