Skip to content

fix: allow retrying failed connection pool stream closures - #1330

Open
AdityaSreevatsaK wants to merge 12 commits into
pydantic:mainfrom
AdityaSreevatsaK:fix/pool-stream-close-retry-1328
Open

AdityaSreevatsaK wants to merge 12 commits into
pydantic:mainfrom
AdityaSreevatsaK:fix/pool-stream-close-retry-1328

Conversation

@AdityaSreevatsaK

@AdityaSreevatsaK AdityaSreevatsaK commented Nov 11, 2026 •

Copy link
Copy Markdown

Summary

Fixes #1328.

PoolByteStream previously marked itself as closed before closing the underlying stream. If that operation raised an exception, subsequent close attempts were skipped, leaving the request registered in the connection pool.

This PR fixes that behaviour for both synchronous and asynchronous streams by resetting _closed when the underlying close fails, allowing a later attempt to retry.

Pool cleanup still occurs only after the underlying stream closes successfully.

Testing:

  • Added regression tests for sync and async streams.
  • Covered repeated failures, retries, BaseException and idempotent cleanup.
  • All 2,125 tests passed across Python 3.11 to 3.15.
  • Achieved 100% test coverage.
  • Ruff, MyPy, build and consistency checks passed.

[Successful CI run](https://github.com/AdityaSreevatsaK/httpx2/actions/runs/43093935454)

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've reviewed the documentation requirements. No documentation changes are needed because this is an internal bug fix with no public API changes.

View guided diff

@veria-ai

veria-ai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR overview

No security issues were found in this pull request.

Reviews

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files

Reply to a comment to ask cubic a question or push back. It learns from your replies.

View guided diff | Re-trigger cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 7 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Re-trigger cubic

Avoid Ruff B009 violations from getattr on string literals in
synchronous and asynchronous test variants.
@codspeed

codspeed Bot commented Nov 12, 2026 •

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 18 untouched benchmarks
⏩ 7 skipped benchmarks1


Comparing AdityaSreevatsaK:fix/pool-stream-close-retry-1328 (7c669cf) with main (f4f65c1)

Open in CodSpeed

Footnotes

  1. 7 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

Prevent simulated close errors from masking request setup failures
in the real HTTP/1.1 and HTTP/2 regression tests.
Core Request objects do not automatically synthesise a Host header,
which HTTP/1.1 and HTTP/2 require for direct pool requests.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A failed close in PoolByteStream latches _closed and keeps the pool slot

1 participant