-
Notifications
You must be signed in to change notification settings - Fork 138
Wait for the Dapr health check asynchronously in aio/clients/grpc/subscription.py to avoid blocking, ensuring the asyncio gRPC stream can close properly. #839
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
4ede53a to
1157957
Compare
5a63471 to
edba7ea
Compare
Switch Dapr health check from blocking call to async call to avoid blocking the event loop in async environments Signed-off-by: mingsing <[email protected]>
Signed-off-by: mingsing <[email protected]>
Signed-off-by: mingsing <[email protected]>
Signed-off-by: mingsing <[email protected]>
edba7ea to
afe00ee
Compare
Signed-off-by: mingsing <[email protected]>
Signed-off-by: mingsing <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #839 +/- ##
==========================================
+ Coverage 86.63% 87.35% +0.72%
==========================================
Files 84 94 +10
Lines 4473 6176 +1703
==========================================
+ Hits 3875 5395 +1520
- Misses 598 781 +183 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: mingsing <[email protected]>
04f0e0d to
c8063fa
Compare
Signed-off-by: mingsing <[email protected]>
c8063fa to
d72b22e
Compare
Signed-off-by: mingsing <[email protected]>
Signed-off-by: mingsing <[email protected]>
Signed-off-by: mingsing <[email protected]>
dapr/aio/clients/grpc/client.py
Outdated
| ) | ||
| from dapr.clients.grpc._state import StateItem, StateOptions | ||
| from dapr.clients.health import DaprHealth | ||
| from dapr.aio.clients.health import DaprHealth |
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.
Sorry for my previous comment, but since we're calling the health check from the __init__, we can't make it asynchronously because __init__ doesn't support async. So I'm afraid we need to stick with the synchronous health check here unfortunately.
It'd be good to have some sort of .connect method that we can call outside of the __init__ and would support asynchronous calls, but there's no such a thing right now, and I don't think we want to make a big change for this...
So, can you bring back the sync call please?
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.
Got it—thanks for the clarification! I've reverted to the synchronous health check.
…secure" Signed-off-by: mingsing <[email protected]>
|
The linter is complaining, can you run |
Switch Dapr health check from blocking call to async call to avoid blocking the event loop in async environments
Description
This PR addresses a critical issue in the Dapr Python SDK's async pubsub subscription functionality where the gRPC stream fails to close properly during reconnection, causing the first response after reconnection to be lost. The root cause is the synchronous health check blocking the async event loop, which prevents the _StreamRequestMixin._done_writing() method from executing properly.
Details:
Solution:
The fix makes the health check asynchronous by using loop.run_in_executor(), which:
Issue reference
We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: