Fix race condition and deadlock in DefaultShard.SendAsync - #136
Closed
Quahu wants to merge 4 commits into
Closed
Conversation
- Added `RequiresAuthentication` and `IsHandshake` helper methods. - `SendAsync` now waits for `Ready` state if the payload requires authentication (e.g., UpdatePresence, RequestMembers). - `SendAsync` no longer waits for `Ready` state if a handshake payload (Identify, Resume) fails, preventing a deadlock where the handshake waits for itself to complete. - This resolves issues where payloads requiring authentication were sent before the session was ready, causing immediate disconnection. Co-authored-by: Quahu <24848652+Quahu@users.noreply.github.com>
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
- Implemented a `SemaphoreSlim` (`_sendLock`) to synchronize `SendAsync` with `InternalRunAsync`. - `SendAsync` now acquires the lock for application payloads (requiring authentication) to ensure the gateway state is stable and Ready. - `InternalRunAsync` acquires the lock when transitioning to a non-Ready state or closing the gateway, preventing concurrent sends during disconnection/reconnection. - Handshake payloads (`Identify`, `Resume`) and `Heartbeat` bypass the lock to avoid deadlocks and ensure connectivity maintenance. - Added `RequiresAuthentication` and `IsHandshake` helpers. - `SendAsync` loops and waits for `Ready` state if the lock acquisition reveals the state has changed. Co-authored-by: Quahu <24848652+Quahu@users.noreply.github.com>
- Implemented a `SemaphoreSlim` (`_sendLock`) to synchronize `SendAsync` with `InternalRunAsync`. - `SendAsync` now acquires the lock for application payloads (requiring authentication) to ensure the gateway state is stable and Ready. - `InternalRunAsync` acquires the lock when transitioning to a non-Ready state or closing the gateway, preventing concurrent sends during disconnection/reconnection. - Handshake payloads (`Identify`, `Resume`) and `Heartbeat` bypass the lock to avoid deadlocks and ensure connectivity maintenance. - Added `RequiresAuthentication` and `IsHandshake` helpers. - `SendAsync` loops and waits for `Ready` state if the lock acquisition reveals the state has changed. - Cleaned up internal implementation details and comments. Co-authored-by: Quahu <24848652+Quahu@users.noreply.github.com>
- Implemented `RequiresAuthentication` and `IsHandshake` helpers to classify payload operations. - Modified `SendAsync` to strictly wait for `Ready` state before sending payloads that require authentication (e.g., UpdatePresence, RequestMembers). - Updated `SendAsync` retry logic to bypass waiting for `Ready` state if the failed payload is a handshake operation (Identify, Resume, Heartbeat), preventing deadlocks. - Simplified synchronization logic by removing unnecessary Semaphores and relying on existing `WaitForReadyAsync` and task completion sources. - This resolves issues where authenticated payloads were sent prematurely during reconnection, causing immediate disconnection. Co-authored-by: Quahu <24848652+Quahu@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR addresses a race condition and potential deadlock in
DefaultShard.SendAsync.The issue involved:
UpdatePresence) being sent before the shard was authenticated, leading to immediate disconnection by the gateway withNotAuthenticated.Identify) waiting for the shard to beReadyupon failure, causing a deadlock becauseIdentifyis required to make the shardReady.The fix:
RequiresAuthentication(GatewayPayloadOperation op)to identify payloads that must wait for theReadystate.IsHandshake(GatewayPayloadOperation op)to identify payloads that should not wait for theReadystate on failure.SendAsyncto enforceWaitForReadyAsyncfor authenticated payloads.SendAsync's catch block to bypassWaitForReadyAsyncfor handshake payloads, allowing the connection loop to retry instead of deadlocking.PR created automatically by Jules for task 16641127480768376837 started by @Quahu