Skip to content

DefaultHttpResponse.StartAsync without feature #7778

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
JamesNK opened this issue Feb 21, 2019 · 7 comments
Closed

DefaultHttpResponse.StartAsync without feature #7778

JamesNK opened this issue Feb 21, 2019 · 7 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

@JamesNK
Copy link
Member

JamesNK commented Feb 21, 2019

If there is no IHttpResponseStartFeature then StartAsync will call Body.FlushAsync every time. Should it only call Body.FlushAsync if HasStarted is false?

https://github.com/aspnet/AspNetCore/blob/25f1f593783de4819a1e8e30dc09af7388cc68b3/src/Http/Http/src/Internal/DefaultHttpResponse.cs#L147-L155

@jkotalik
Copy link
Contributor

Yeah I'd be fine with that change.

@jkotalik jkotalik self-assigned this Feb 21, 2019
@jkotalik jkotalik added this to the 3.0.0-preview3 milestone Feb 21, 2019
@jkotalik
Copy link
Contributor

Going to put this in preview3 for now (it's a 1 line fix). @muratg are we still fine to merge into preview3 or does it require approval?

@muratg
Copy link
Contributor

muratg commented Feb 21, 2019

@jkotalik would require approval. cc @Eilon

@Eilon
Copy link
Member

Eilon commented Feb 21, 2019

Why do we need it in preview3? What doesn't work that would affect a customer trying preview3?

@jkotalik
Copy link
Contributor

jkotalik commented Feb 21, 2019 via email

@muratg muratg modified the milestones: 3.0.0-preview3, 3.0.0-preview4 Feb 21, 2019
@Eilon
Copy link
Member

Eilon commented Feb 21, 2019

If it's not a critical need or preview 3 then I wouldn't bother and just go with master.

@jkotalik
Copy link
Contributor

We decided to keep the flush behavior, but we should still check IsStarted too.

@jkotalik jkotalik added feature-pipelines Anything relating to exposing/using Pipes in ASP.NET Core cost: XS labels Feb 21, 2019
@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

6 participants