Fix TestServer from blocking on request stream #15591

JamesNK · 2019-10-27T03:59:16Z

HttpContent.ReadAsStreamAsync() will block when called in custom HttpContent implementations. Calling HttpContent.CopyToAsync(stream) mirrors what HttpClient does more closely.

Note, this PR is on 3.1. The bug blocks people from using TestServer with gRPC. Would be nice to unblock before 5.0. @anurse is this your call? Need anymore info?

davidfowl · 2019-10-27T03:59:45Z

Do it in 5.0 then backport to 3.1?

src/Hosting/TestHost/src/ClientHandler.cs

src/Hosting/TestHost/test/Utilities.cs

src/Hosting/TestHost/src/ClientHandler.cs

Tratcher · 2019-10-27T15:14:28Z

HttpContent.ReadAsStreamAsync() will block when called in custom HttpContent implementations.

Which custom HttpContent implementations? It sounds like the content objects are at fault here. I'm not sure why you're changing the server to accommodate them.

src/Hosting/TestHost/src/ClientHandler.cs

Tratcher · 2019-10-27T15:41:50Z

Do it in 5.0 then backport to 3.1?

Forward is easier than backwards.

davidfowl · 2019-10-27T16:41:25Z

Which custom HttpContent implementations? It sounds like the content objects are at fault here. I'm not sure why you're changing the server to accommodate them.

Sorta but not really. It's much more complicated to implement this in each HttpContent. See https://github.com/dotnet/corefx/blob/26d7b6626ab0385a1158e674855f81ab73f628e0/src/System.Net.Http/src/System/Net/Http/HttpContent.cs#L453

I had the same impression at first but it's far easier to make this work in TestServer than it is to fix the HttpContent implementation (just like we auto-rewind your Stream, though I don't, agree with that one).

Forward is easier than backwards.

But then we can't merge it until it goes through the layers of approval. But whatever works I guess.

JamesNK · 2019-10-27T19:17:23Z

Which custom HttpContent implementations?

https://github.com/grpc/grpc-dotnet/blob/9e39edb0a3fc7a625085490bf64c2794870a519e/src/Grpc.Net.Client/Internal/PushStreamContent.cs

It sounds like the content objects are at fault here. I'm not sure why you're changing the server to accommodate them.

No TestServer is at fault for using HttpContent.ReadAsStreamAsync instead of HttpContent.CopyToAsync. HttpContent.ReadAsStreamAsync is never called on the request content by HttpClientHandler.

Should the gRPC content fix itself so ReadAsStreamAsync is supported without blocking? Maybe. But that's a separate issue.

JamesNK · 2019-10-27T20:06:47Z

Related question in HttpClient-side - https://github.com/dotnet/corefx/issues/42160

Tratcher · 2019-10-27T21:22:44Z

No TestServer is at fault for using HttpContent.ReadAsStreamAsync instead of HttpContent.CopyToAsync. HttpContent.ReadAsStreamAsync is never called on the request content by HttpClientHandler.

Should the gRPC content fix itself so ReadAsStreamAsync is supported without blocking? Maybe. But that's a separate issue.

Content that expect to stream really should implement CreateContentReadStreamAsync and then you wouldn't have to change anything in TestServer, no?

davidfowl · 2019-10-27T22:11:10Z

This change to TestServer is fine

JamesNK · 2019-10-28T23:57:54Z

/cc @Pilchie / @anurse for 3.1.0-preview3 consideration

JamesNK · 2019-10-29T00:19:58Z

Also, looking for review approvals // @davidfowl, @Tratcher

analogrelay · 2019-10-29T02:16:54Z

TestServer is a pretty low-risk area. I'm in favor for 3.1. It's @Pilchie 's final call though.

src/Hosting/TestHost/src/ClientHandler.cs

src/Hosting/TestHost/src/HttpContextBuilder.cs

src/Hosting/TestHost/src/ClientHandler.cs

JamesNK · 2019-10-29T22:22:50Z

🆙 📅

src/Hosting/TestHost/src/HttpContextBuilder.cs

src/Hosting/TestHost/test/TestClientTests.cs

Pilchie · 2019-10-29T23:58:54Z

What is the scenario for TestHost is it (as the name implies) purely for testing, or is it something that customers use on a regular basis in production that we could break?

analogrelay · 2019-10-30T00:49:31Z

What is the scenario for TestHost is it (as the name implies) purely for testing, or is it something that customers use on a regular basis in production that we could break?

It's for testing only. Using it in a production is not supported. It doesn't ship in any shared framework.

Essentially it's an in-memory test server. You can point it at your app setup (middlewares, services, etc.) and it will start up a fake "app" and give you an HttpClient that can make fake "requests" to your app.

davidfowl · 2019-10-30T04:30:04Z

What is the scenario for TestHost is it (as the name implies) purely for testing, or is it something that customers use on a regular basis in production that we could break?

People use it in Azure functions to bootstrap ASP.NET Core on top of Azure functions.

src/Hosting/TestHost/src/HttpContextBuilder.cs

Tratcher · 2019-10-30T17:08:44Z

src/Hosting/TestHost/src/HttpContextBuilder.cs

+            configureContext(_httpContext, _requestPipe.Reader);
+        }
+
+        internal void SendRequestStream(Func<PipeWriter, Task> sendRequestStream)


The layering still seems odd here. Only ClientHandler needs/uses the request body pipe and _sendRequestStreamTask, why doesn't it own those? What it needs from HttpContextBuilder is a callback for CompleteRequestAsync and/or cancel.

I did it this way because the alternative is callbacks everywhere:

After request is complete

Request is aborted from server

Request is aborted from client

Sure HttpContextBuilder owns the request pipe and task, but using it is optional.

Yea I don’t think this layering is super important

Pilchie · 2019-10-30T19:49:41Z

Ok, Approved for 3.1.0-Preview3.

JamesNK · 2019-10-31T19:38:29Z

Is there any more feedback/changes required here?

JamesNK requested review from davidfowl and Tratcher October 27, 2019 03:59

JamesNK requested a review from analogrelay as a code owner October 27, 2019 03:59

davidfowl reviewed Oct 27, 2019

View reviewed changes

src/Hosting/TestHost/src/ClientHandler.cs Outdated Show resolved Hide resolved

davidfowl reviewed Oct 27, 2019

View reviewed changes

src/Hosting/TestHost/test/Utilities.cs Show resolved Hide resolved

davidfowl reviewed Oct 27, 2019

View reviewed changes

src/Hosting/TestHost/src/ClientHandler.cs Outdated Show resolved Hide resolved

JamesNK added the 3.1-candidate label Oct 27, 2019

Tratcher reviewed Oct 27, 2019

View reviewed changes

src/Hosting/TestHost/src/ClientHandler.cs Outdated Show resolved Hide resolved

Pilchie added the area-servers label Oct 28, 2019

Tratcher requested changes Oct 29, 2019

View reviewed changes

Support duplex streaming in TestServer

2ee8f82

JamesNK force-pushed the jamesnk/testserver-clientstreaming branch from b4a7f55 to 2ee8f82 Compare October 29, 2019 22:17

Tratcher reviewed Oct 29, 2019

View reviewed changes

src/Hosting/TestHost/src/HttpContextBuilder.cs Outdated Show resolved Hide resolved

src/Hosting/TestHost/test/TestClientTests.cs Outdated Show resolved Hide resolved

src/Hosting/TestHost/test/TestClientTests.cs Outdated Show resolved Hide resolved

PR feedback

284fc04

davidfowl reviewed Oct 30, 2019

View reviewed changes

src/Hosting/TestHost/src/HttpContextBuilder.cs Outdated Show resolved Hide resolved

PR feedback

d5119e6

Tratcher reviewed Oct 30, 2019

View reviewed changes

PR feedback

df21112

Tratcher approved these changes Oct 31, 2019

View reviewed changes

JamesNK merged commit cfea2e9 into release/3.1 Oct 31, 2019

JamesNK deleted the jamesnk/testserver-clientstreaming branch October 31, 2019 21:48

JamesNK added this to the 3.1.0-preview3 milestone Nov 3, 2019

JamesNK mentioned this pull request Nov 3, 2019

Bidirectional streaming regression on TestServer #15415

Closed

Tratcher mentioned this pull request Nov 5, 2019

TestServer: Content-Length header not added for Posts #16822

Closed

JamesNK mentioned this pull request Dec 6, 2019

Fix TestServer hang with duplex streaming requests #17158

Merged

JamesNK mentioned this pull request Feb 1, 2020

Streaming HttpContent and ReadAsStreamAsync dotnet/runtime#31316

Closed

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TestServer from blocking on request stream #15591

Fix TestServer from blocking on request stream #15591

JamesNK commented Oct 27, 2019 •

edited

Loading

davidfowl commented Oct 27, 2019

Tratcher commented Oct 27, 2019

Tratcher commented Oct 27, 2019

davidfowl commented Oct 27, 2019

JamesNK commented Oct 27, 2019

JamesNK commented Oct 27, 2019

Tratcher commented Oct 27, 2019

davidfowl commented Oct 27, 2019

JamesNK commented Oct 28, 2019

JamesNK commented Oct 29, 2019

analogrelay commented Oct 29, 2019

JamesNK commented Oct 29, 2019

Pilchie commented Oct 29, 2019

analogrelay commented Oct 30, 2019 •

edited

Loading

davidfowl commented Oct 30, 2019

Tratcher Oct 30, 2019

JamesNK Oct 30, 2019

davidfowl Oct 30, 2019

Pilchie commented Oct 30, 2019

JamesNK commented Oct 31, 2019

Fix TestServer from blocking on request stream #15591

Fix TestServer from blocking on request stream #15591

Conversation

JamesNK commented Oct 27, 2019 • edited Loading

davidfowl commented Oct 27, 2019

Tratcher commented Oct 27, 2019

Tratcher commented Oct 27, 2019

davidfowl commented Oct 27, 2019

JamesNK commented Oct 27, 2019

JamesNK commented Oct 27, 2019

Tratcher commented Oct 27, 2019

davidfowl commented Oct 27, 2019

JamesNK commented Oct 28, 2019

JamesNK commented Oct 29, 2019

analogrelay commented Oct 29, 2019

JamesNK commented Oct 29, 2019

Pilchie commented Oct 29, 2019

analogrelay commented Oct 30, 2019 • edited Loading

davidfowl commented Oct 30, 2019

Tratcher Oct 30, 2019

Choose a reason for hiding this comment

JamesNK Oct 30, 2019

Choose a reason for hiding this comment

davidfowl Oct 30, 2019

Choose a reason for hiding this comment

Pilchie commented Oct 30, 2019

JamesNK commented Oct 31, 2019

JamesNK commented Oct 27, 2019 •

edited

Loading

analogrelay commented Oct 30, 2019 •

edited

Loading