Skip to content

Keep Pipe readable after FormPipeReader error#18939

Merged
analogrelay merged 1 commit intorelease/3.1from
halter73/17723-3.1
Feb 14, 2020
Merged

Keep Pipe readable after FormPipeReader error#18939
analogrelay merged 1 commit intorelease/3.1from
halter73/17723-3.1

Conversation

@halter73
Copy link
Member

@halter73 halter73 commented Feb 10, 2020

When FormPipeReader.ReadFormAsync() threw due to some limit being exceeded, it would not call PipeReader.AdvanceTo() which can leave the request body pipe in an unusable state. We should also consider changes to Kestrel to improve resiliency against this across requests, but this is the simpler fix.

Fixes #17723

@anurse I think we should take this as a 3.1 servicing patch.

@halter73 halter73 added the Servicing-consider Shiproom approval is required for the issue label Feb 10, 2020
@halter73 halter73 added this to the 3.1.x milestone Feb 10, 2020
@ghost ghost added the area-servers label Feb 10, 2020
@analogrelay analogrelay removed the Servicing-consider Shiproom approval is required for the issue label Feb 11, 2020
@analogrelay
Copy link
Contributor

Please make sure to fill in the template before putting servicing-consider on (I'll put it in a wiki page or something):

#### Description

<!-- Describe the bug and the fix. -->

#### Customer Impact

<!--
* Was the bug reported by a customer?
* How does the bug impact the customer
* Are there any workarounds? If so, why are they not acceptable alternatives?
-->

#### Regression?

<!-- Either "no", or include the version that regressed the behavior -->

#### Risk

<!-- Describe the risk of introducing this change. -->

@halter73
Copy link
Member Author

Description

Prior to this fix, when FormPipeReader.ReadFormAsync() threw due to some limit being exceeded, it would not call PipeReader.AdvanceTo() which can leave the request body pipe in an unusable state. This can interfere with Kestrel's ability to process the next request on the connection causing Kestrel to abruptly close the connection. We should also consider changes to Kestrel to improve resiliency against this across requests, but this is the simpler fix.

Customer Impact

  • Was the bug reported by a customer?
  • How does the bug impact the customer?
    • "This problem causes the connection to our proxy to break, which the proxy interprets as 500."
  • Are there any workarounds? If so, why are they not acceptable alternatives?
    • Catching the exception and setting the "Connection: close" header might work around the issue, but this hasn't been tested with the proxy in question, and would hurt performance since the proxy would need to establish a new connection each time this happens.

Regression?

Yes. This bug did not exist prior to 3.0.

In 3.0, we started using FormPipeReader instead of the Stream-based FormReader from the FormFeature.

Risk

Very minimal. It's catching and rethrowing an exception after putting the request body Pipe back in a readable state.

@analogrelay analogrelay added the Servicing-consider Shiproom approval is required for the issue label Feb 12, 2020
@analogrelay
Copy link
Contributor

Thanks @halter73 !

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Feb 13, 2020
@leecow leecow modified the milestones: 3.1.x, 3.1.3 Feb 13, 2020
@analogrelay analogrelay added Servicing-approved Shiproom has approved the issue and removed Servicing-approved Shiproom has approved the issue labels Feb 13, 2020
@analogrelay analogrelay merged commit f3e2b4d into release/3.1 Feb 14, 2020
@analogrelay analogrelay deleted the halter73/17723-3.1 branch February 14, 2020 17:36
@amcasey 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
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 Servicing-approved Shiproom has approved the issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants