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

Still send 100 Continue given null MinRequestBodyDataRate #31284

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Mar 26, 2021

Description

This fixes a bug in Kestrel where a non-default configuration MinRequestBodyDataRate = null causes Kestrel not to send a "100 Continue" response given a "Expect: 100-continue" request header.

Customer impact

This was found (and eventually debugged) by an internal customer.

I am trying to migrate our code base from using Kestrel 2.2 (PackageReference 2.2.0 running on runtime 3.1.12) to Kestrel 3.1 (FrameworkReference onto Kestrel 3.1 with Runtime 3.1.12).
When running our performance tests, we saw a dramatic reduction in throughput, increase in latency and a precipitous drop in CPU – overall reduction in performance for POST workloads
...
If I measure the Stopwatch time to do a 0-byte read of the request body in the beginning of application (server) middleware code, the 2.2 code takes around 3ms, while the 3.1 code takes 15-20ms.

And it was also reported externally: #21502.

This shows up as a performance regression rather than failed request because clients will eventually start uploading the request body without a "100 Continue" response from Kestrel, but it will wait for a timeout causing increased latency.

Not all clients use "Expect: 100-continue" request header, but older .NET Framework clients do.

Regresssion

Yes. This was a regression introduced in 3.0.

Risk

Low. MinRequestBodyDataRate should have never had any impact on whether or not Kestrel sends a "100 Continue" response.

Fixes #30449

@halter73 halter73 added bug This issue describes a behavior which is not expected - a bug. Servicing-consider Shiproom approval is required for the issue area-servers feature-kestrel labels Mar 26, 2021
@halter73 halter73 added this to the 3.1.x milestone Mar 26, 2021
@halter73 halter73 marked this pull request as ready for review March 26, 2021 16:28
@halter73 halter73 requested a review from jkotalik as a code owner March 26, 2021 16:28
@ghost
Copy link

ghost commented Mar 26, 2021

Hello human! Please make sure you've included the Shiproom Template in a comment or (preferably) the PR description. Also, make sure this PR is not marked as a draft and is ready-to-merge.

@halter73 halter73 removed the bug This issue describes a behavior which is not expected - a bug. label Mar 26, 2021
@Pilchie
Copy link
Member

Pilchie commented Apr 1, 2021

@halter73 / @davidfowl can one of you fill out the Ask Mode template so we can talk about this at Tactics today?

@halter73
Copy link
Member Author

halter73 commented Apr 1, 2021

@Pilchie I've updated the description.

@Pilchie
Copy link
Member

Pilchie commented Apr 1, 2021

Thanks - we missed Tactics this morning, but we'll talk about it on Tuesday.

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Apr 6, 2021
@leecow leecow modified the milestones: 3.1.x, 3.1.15 Apr 6, 2021
@Pilchie
Copy link
Member

Pilchie commented Apr 6, 2021

@halter73 any chance we get get the internal customer to verify the fix with a private before we merge, or at least before we ship?

Also, do we need a port of this to 5.0?

@halter73
Copy link
Member Author

halter73 commented Apr 6, 2021

I asked the customer if they can verify the fix and cc'd you to the thread @Pilchie. And I opened the 5.0 PR at #31568.

@wtgodbe wtgodbe merged commit 5e41934 into release/3.1 Apr 7, 2021
@wtgodbe wtgodbe deleted the halter73/30449 branch April 7, 2021 23:06
@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 join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-kestrel Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants