Skip to content

Dispose the prior response on retry #36162

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.

Sign up for GitHub

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

Jump to bottom
Merged
Tratcher merged 1 commit into from
Sep 9, 2021
Merged

Dispose the prior response on retry #36162

Tratcher merged 1 commit into from
Sep 9, 2021

Conversation

@Tratcher
Copy link
Member

@Tratcher Tratcher commented Sep 3, 2021

Fixes #28384
6.0 rc2 candidate: resource leak / hang

  • The HttpResponseMessage needs to be buffered, drained, or disposed to free up the connection for the next request.
  • Polly is a generic policy library, it's not aware of HttpRequest/ResponseMessage lifetimes or the need to dispose them on retry. Polly.Extensions.Http is aware of HttpRequest/ResponseMessage but doesn't participate in the retry execution, only the decision on if a retry is recommended.

Fix: Add a reference from the request to the response so that if the request is retried, it can dispose the prior response.

@Tratcher Tratcher added area-runtime feature-httpclientfactory Includes: HttpClientFactory (some bugs also in Extensions repo) labels Sep 3, 2021
@Tratcher Tratcher added this to the 7.0-preview1 milestone Sep 3, 2021
@Tratcher Tratcher requested a review from rynowak September 3, 2021 21:31
@Tratcher Tratcher self-assigned this Sep 3, 2021
@davidfowl davidfowl enabled auto-merge (squash) September 8, 2021 15:52
@davidfowl davidfowl disabled auto-merge September 8, 2021 15:52
@Tratcher Tratcher merged commit 8d5212d into dotnet:main Sep 9, 2021
@Tratcher Tratcher deleted the tratcher/clientfactory/dispose branch September 9, 2021 00:21
@Tratcher
Copy link
Member Author

Tratcher commented Sep 9, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

github-actions bot commented Sep 9, 2021

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1215403707

@github-actions github-actions bot mentioned this pull request Sep 9, 2021
Merged
This was referenced Oct 13, 2021
Merged
Merged
@martincostello martincostello mentioned this pull request Jan 30, 2022
Merged
@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

Reviewers

@captainsafia captainsafia captainsafia approved these changes

@rynowak rynowak Awaiting requested review from rynowak

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

Assignees

@Tratcher Tratcher

Labels

area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions feature-httpclientfactory Includes: HttpClientFactory (some bugs also in Extensions repo)

Projects

None yet

Milestone

7.0-preview1

Development

Successfully merging this pull request may close these issues.

Microsoft.Extensions.Http.Polly: Deadlock when retrying a "503 - Service unavailable"

3 participants

@Tratcher @captainsafia @amcasey