-
Notifications
You must be signed in to change notification settings - Fork 5
Fix async client timeout and cancellation issues #77
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.
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
base: main
Are you sure you want to change the base?
Conversation
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]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
Signed-off-by: Anuraag Agrawal <[email protected]>
7eac0a8 to
49562fc
Compare
…nto conformance-client-parallel
Signed-off-by: Anuraag Agrawal <[email protected]>
|
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", |
There was a problem hiding this comment.
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
stefanvanburen
left a comment
There was a problem hiding this 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)?
Uh oh!
There was an error while loading. Please reload this page.