Skip to content

Conversation

@anuraaga
Copy link
Collaborator

@anuraaga anuraaga commented Jan 7, 2026

  • Shield httpx response close from timeout so its closing of response iterators happens without interruption
  • Separate tasks for executing httpx requests and retrieve responses through a queue
    • httpx does not seem to reliably cancel I/O, so to be able to return from the client quickly when cancelled, this seems like the only pattern to allow it. In some cases the I/O will still make it through to completion, but at least the client isn't blocked on it
  • Improve timing of cancel_after_close to be as close to after close as possible. Before it was run right away
  • Don't write TLS certs to files on event loop which would block it. I found it while seeing if it was just a busy event loop causing flakiness before the above findings
  • Run client conformance tests in parallel to speed them up and do some stress testing of the client, which is what exposed the above bugs

@anuraaga anuraaga requested a review from a team January 7, 2026 03:40
@anuraaga anuraaga changed the title Run conformance client tests in parallel Prevent cancelling httpx async stream cleanup Jan 7, 2026
@anuraaga anuraaga marked this pull request as draft January 7, 2026 05:13
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
@anuraaga anuraaga force-pushed the conformance-client-parallel branch from 7eac0a8 to 49562fc Compare January 8, 2026 05:56
@anuraaga anuraaga changed the title Prevent cancelling httpx async stream cleanup Fix async client timeout and cancellation issues Jan 8, 2026
@anuraaga anuraaga marked this pull request as ready for review January 8, 2026 06:07
@anuraaga
Copy link
Collaborator Author

anuraaga commented Jan 8, 2026

Ok @connectrpc/python sorry for the churn. Initially all I wanted was to speed up client conformance tests, but that also ended up being a stress test of the client which we haven't really done before (it's only num cpus * 4, in the future we may need even more stressful tests) - this showed some possible bugs that could surface to users, albeit for corner cases. And while I was at it, we already had client cancellation tests marked as flaky, but I could finally deep dive into them and I think they're actually fixed, at least as best as we can do.

The change is large and complicated so it will be good to get eyes on it - @spenczar especially, I wonder if you've ran into patterns for dealing with async and cancellations cleanly before, maybe there's something besides the producer/consumer approach I took.

*_skipped_tests_async,
"--known-flaky",
"Client Cancellation/**",
"**/cancel-after-responses",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is still flaky, I suspect it is an issue in the test harness, not code. I will defer debugging it as already did a lot of debugging and want to get the fixes in

@anuraaga anuraaga mentioned this pull request Jan 9, 2026
Copy link
Member

@stefanvanburen stefanvanburen left a comment

Choose a reason for hiding this comment

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

stamping so we can get this in for #79; looks reasonable to me but not as familiar with the asyncio.Queue approach so up to you if you want to wait for a more thorough review.

httpx does not seem to reliably cancel I/O

Is there an upstream issue we can track? Or ought we open one (maybe hard to pinpoint)?

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.

2 participants