Skip to content
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

Remove Kestrel Pipe pause and resume thresholds and be more preemptive about sending large available frame sizes. #8105

Closed
jkotalik opened this issue Mar 1, 2019 · 4 comments
Assignees
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Done This issue has been fixed feature-kestrel

Comments

@jkotalik
Copy link
Contributor

jkotalik commented Mar 1, 2019

We hit this issue with #7964. The fundamental issue is that if we are calling context.Request.BodyPipe.ReadAsync() and then call AdvanceTo(buffer.Start, buffer,End) (examine without consuming), we will eventually get to a point where we buffer PipeReader too far and it would cause backpressure OR in the case of HTTP2 send a frame update saying that the client shouldn't send more data. We decided that this shouldn't be fixed by the parsers and instead should be fixed in Kestrel.

A few things will happen between Kestrel and Pipelines

  • The pause and resume thresholds should be removed and limits should be enforced in parsers.
  • When calling PipeWriter.AdvanceTo, it should never throw where there is backpressure and the thresholds should be a soft limit. In this case, we can keep the limits in Kestrel.
@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 1, 2019

cc @davidfowl @Tratcher

@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 1, 2019

We probably want this in preview4, correct?

@Tratcher
Copy link
Member

Tratcher commented Mar 1, 2019

Yes, the form reader needs this.

HTTP/2 clarification: When there's backpressure it's signaled via the absence of window update frames, not by a special frame asking the client to stop.

  • Change: Send window updates based on Examined rather than Consumed (and remove the buffer limits).

@jkotalik
Copy link
Contributor Author

jkotalik commented Mar 1, 2019

Ah didn't realize that the lack of window updates is what causes the client to stop.

@jkotalik jkotalik self-assigned this Mar 4, 2019
@davidfowl davidfowl added Done This issue has been fixed and removed 2 - Working labels Mar 13, 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 Done This issue has been fixed feature-kestrel
Projects
None yet
Development

No branches or pull requests

5 participants