Skip to content

TestServer needs a IHttpResponseStart feature to avoid flushing #7780

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

Closed
JunTaoLuo opened this issue Feb 21, 2019 · 9 comments
Closed

TestServer needs a IHttpResponseStart feature to avoid flushing #7780

JunTaoLuo opened this issue Feb 21, 2019 · 9 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-pipelines Anything relating to exposing/using Pipes in ASP.NET Core

Comments

@JunTaoLuo
Copy link
Contributor

JunTaoLuo commented Feb 21, 2019

To mimic the Kestrel behaviour, a IHttpResponseStartFeature needs to be added by the TestServer. Otherwise it falls back to the flushing behaviour: https://github.com/aspnet/AspNetCore/blob/4646ed56189a8ee36ced6397dd4f86fce9e6a8e8/src/Http/Http/src/Internal/DefaultHttpResponse.cs#L154-L159.

@jkotalik
Copy link
Contributor

So a few questions:

  • Do you agree that flushing if the StartFeature isn't implemented is desired behavior?
  • If so, why do you need TestServer to implement the IHttpResponseStartFeature? Are you trying to verify that StartAsync doesn't flush headers to the client?

I agree that having inconsistent behavior based on if the feature is implemented isn't great. Maybe we should throw if the feature isn't implemented instead.

@JamesNK
Copy link
Member

JamesNK commented Feb 21, 2019

This area came up when when we upgraded gRPC to the latest version of preview 3. A combination of circumstances combined:

  • We had heard that StartAsync is required before using Response.BodyPipe, and that calling it multiple times is fine because the feature will no-op once the response has started
  • We changed our write message code to start with a call to StartAsync
  • We have a unit test that does buffering and calls write message multiple times
  • Because our test is running in TestServer there was no IHttpResponseStartFeature so writing a message would flush previous messages

The main problem here is the difference in behavior between a real server and the TestServer. The test server is flushing each time StartAsync is called. That could be solved by #7778

@Tratcher
Copy link
Member

The fallback behavior is OK, but we should go ahead and implement IHttpResponseStartFeature and pipes. There's already a response body pipe.

@jkotalik jkotalik changed the title TestServer flushes when StartAsync is called Non-Kestrel servers flushes when StartAsync is called Feb 21, 2019
@jkotalik jkotalik added this to the 3.0.0-preview4 milestone Feb 21, 2019
@jkotalik jkotalik self-assigned this Feb 21, 2019
@jkotalik jkotalik added the feature-pipelines Anything relating to exposing/using Pipes in ASP.NET Core label Feb 21, 2019
@Tratcher
Copy link
Member

#7815

@muratg
Copy link
Contributor

muratg commented Feb 27, 2019

Triage: We won't be doing this. Instead we'll do #7779

@muratg muratg closed this as completed Feb 27, 2019
@JunTaoLuo
Copy link
Contributor Author

Why are we not adding the feature to TestServer?

@JunTaoLuo JunTaoLuo changed the title Non-Kestrel servers flushes when StartAsync is called TestServer needs a IHttpResponseStart feature to avoid flushing Feb 27, 2019
@JunTaoLuo
Copy link
Contributor Author

I think I named the issue poorly and it was misunderstood? We should still add the feature to the test server.

@JamesNK
Copy link
Member

JamesNK commented Feb 27, 2019

Related: TestServer should be using real pipes instead of the stream adapters

@jkotalik
Copy link
Contributor

#7815

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-pipelines Anything relating to exposing/using Pipes in ASP.NET Core
Projects
None yet
Development

No branches or pull requests

7 participants