-
Notifications
You must be signed in to change notification settings - Fork 72
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
Wait for All Callbacks in _emit
to Finish, Add More Thread Tracing
#415
Wait for All Callbacks in _emit
to Finish, Add More Thread Tracing
#415
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant AsyncLiveClient
participant EventHandler
participant asyncio
Client->>AsyncLiveClient: Initialize
AsyncLiveClient->>AsyncLiveClient: _listen_thread = None
AsyncLiveClient->>AsyncLiveClient: _keep_alive_thread = None
Client->>AsyncLiveClient: Start listening
AsyncLiveClient->>AsyncLiveClient: Create _listen_thread (asyncio.create_task)
AsyncLiveClient->>AsyncLiveClient: Log active threads
Client->>AsyncLiveClient: Keep alive
AsyncLiveClient->>AsyncLiveClient: Create _keep_alive_thread (asyncio.create_task)
AsyncLiveClient->>AsyncLiveClient: Log active threads
Client->>AsyncLiveClient: Emit event
AsyncLiveClient->>EventHandler: Create event handler tasks (asyncio.create_task)
EventHandler->>AsyncLiveClient: Return task
AsyncLiveClient->>asyncio: Gather tasks (asyncio.gather)
asyncio->>AsyncLiveClient: Return results
AsyncLiveClient->>Client: Event handled
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
_emit
to Finish, Add More Thread Tracing
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.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
deepgram/clients/live/v1/async_client.py (1)
8-8
: Consider using a more specific import forthreading
if only certain functions are used.This can help reduce the namespace clutter and potentially improve understanding of which threading features are being utilized.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deepgram/clients/live/v1/async_client.py (7 hunks)
- deepgram/clients/live/v1/client.py (5 hunks)
Files skipped from review due to trivial changes (1)
- deepgram/clients/live/v1/client.py
Additional comments not posted (4)
deepgram/clients/live/v1/async_client.py (4)
53-54
: The change to allow_listen_thread
and_keep_alive_thread
to beNone
is a good safety measure.This modification enhances robustness by handling scenarios where these threads might not be initialized yet.
146-150
: Adding debug statements to track thread activity before and after significant operations is a good practice.This will help in diagnosing issues related to thread management and ensure that all threads are accounted for before proceeding.
Also applies to: 161-165
Line range hint
639-660
: Properly handling the cancellation of threads during thefinish
method is crucial.This ensures that all background activities are terminated gracefully, preventing any potential resource leaks or unfinished transactions.
212-229
: Ensure that all tasks created within_emit
are awaited properly.This is crucial to prevent any tasks from being orphaned, which could lead to resource leaks or inconsistent application state.
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.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- deepgram/clients/live/v1/async_client.py (6 hunks)
- deepgram/clients/live/v1/client.py (5 hunks)
Files skipped from review as they are similar to previous changes (2)
- deepgram/clients/live/v1/async_client.py
- deepgram/clients/live/v1/client.py
Proposed changes
Addresses: #408, #409
This PR:
AsyncLiveClient
andLiveClient
to keep track of threadsAsyncLiveClient._emit
, we want to wait for all the callbacks to complete before exiting_emit
. This isn't a bug, provided the user doesn't have a callback function that doesn't hang.Types of changes
What types of changes does your code introduce to the community Python SDK?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
NA
Summary by CodeRabbit