Skip to content

Resolve race condition around exceptions during streaming a response. - #491

Merged
lovelydinosaur merged 8 commits into
masterfrom
defensive-programming-when-removing-request-from-pool
Jan 31, 2022
Merged

Resolve race condition around exceptions during streaming a response.#491
lovelydinosaur merged 8 commits into
masterfrom
defensive-programming-when-removing-request-from-pool

Conversation

@lovelydinosaur

@lovelydinosaurlovelydinosaur commented Jan 27, 2022•
edited
Loading

Copy link
Copy Markdown
Contributor

Closeshttps://github.com/encode/httpx/issues/2047 by resolving a race condition that can occur when an exception is raised while streaming the response.

Prior to this PR it's possible that the exception is handled first, closing the connection, and then the request close callback occurs.

The changes are:

  • If an exception occurs while streaming a response, then close the response (and possibly the connection)
    before allowing the exception to bubble up.
  • If a connection pool is closed while there are still open requests, then close it but then raise a RuntimeError.
  • Defensive programming around removing the request status from the connection pool, when closing the request.

@lovelydinosaurlovelydinosaur added the bug Something isn't working label Jan 27, 2022
@lovelydinosaurlovelydinosaur changed the title Resolve race condition with exceptions that Resolve race condition around exceptions during streaming a response.Jan 27, 2022
ahopkins
ahopkins reviewed Jan 27, 2022
View reviewed changes
Comment threadhttpcore/_async/connection_pool.py Outdated
ahopkins
ahopkins reviewed Jan 27, 2022
View reviewed changes
Comment threadhttpcore/_async/http11.py
@lovelydinosaur

lovelydinosaur commented Jan 28, 2022

Copy link
Copy Markdown
ContributorAuthor

@ahopkins Great thanks - addressed one of your comments. Note that you'll need to accept the @encode/maintainers invitation before your review gets "merge approval" status.

@lovelydinosaurlovelydinosaur mentioned this pull request Jan 28, 2022
Merged
@lovelydinosaur
lovelydinosaur requested a review from a teamJanuary 28, 2022 19:35
@lovelydinosaur
lovelydinosaur merged commit 7ddb4ca into masterJan 31, 2022
@lovelydinosaur
lovelydinosaur deleted the defensive-programming-when-removing-request-from-pool branch January 31, 2022 12:15
@lovelydinosaurlovelydinosaur mentioned this pull request Feb 1, 2022
Merged
jogu added a commit to openid-certification/conformance-suite that referenced this pull request Feb 2, 2022
@jogu
Fix retries to work with httpcore 0.14.6
9b5188b
This change:
encode/httpcore#491
had added an exception when requests were left still open when the
client was closed. It seems I was accidentally doing this when
retrying requests, so add an explicit close before retrying.
jogu added a commit to openid-certification/conformance-suite that referenced this pull request Feb 2, 2022
@jogu
Fix retries to work with httpcore 0.14.6
546a4c0
This change:
encode/httpcore#491
had added an exception when requests were left still open when the
client was closed. It seems I was accidentally doing this when
retrying requests, so add an explicit close before retrying.
@lovelydinosaurlovelydinosaur mentioned this pull request Feb 2, 2022
Merged
@KludexKludex mentioned this pull request May 12, 2026
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@ahopkinsahopkinsahopkins approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugSomething isn't working

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Potential ValueError when used with high number of concurrent coros

2 participants

@lovelydinosaur@ahopkins