-
-
Notifications
You must be signed in to change notification settings - Fork 535
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
Fix result source was never awaited if clients disconnect fast #3687
Fix result source was never awaited if clients disconnect fast #3687
Conversation
Reviewer's Guide by SourceryThis PR fixes a race condition in WebSocket protocol handlers where coroutines could remain unawaited if clients disconnect quickly. The implementation restructures the operation handling flow to ensure that awaitables are handled immediately after creation, preventing any potential gaps where cancellation could leave coroutines unawaited. Sequence diagram for WebSocket operation handlingsequenceDiagram
actor Client
participant Handler as WebSocketHandler
participant Schema
Client->>Handler: Send SubscribeMessage
Handler->>Handler: Create Operation
Handler->>Schema: Subscribe/Execute Query
Schema-->>Handler: Return Awaitable
Handler->>Handler: Await Result Source
alt Client disconnects quickly
Handler->>Handler: Cancel Task
else Client remains connected
Handler->>Client: Send Results
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey @DoctorJohn - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟡 Documentation: 1 issue found
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Thanks for adding the Here's a preview of the changelog: This release fixes the issue that some coroutines in the WebSocket protocol handlers were never awaited if clients disconnected shortly after starting an operation. Here's the tweet text:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3687 +/- ##
==========================================
+ Coverage 96.99% 97.01% +0.01%
==========================================
Files 503 503
Lines 33548 33550 +2
Branches 5634 5634
==========================================
+ Hits 32541 32548 +7
- Misses 795 796 +1
+ Partials 212 206 -6 |
CodSpeed Performance ReportMerging #3687 will not alter performanceComparing Summary
|
Description
While working on #3685, pytest showed me a warning stating
RuntimeWarning: coroutine 'Schema.subscribe' was never awaited
while testing WebSockets. The source of the issue lies in the following sequence:result_source
)result_source
in a taskIf the WebSocket client disconnects after we yield from the event loop and before we await
result_source
, the task will be cancelled and the awaitable will never be awaited. This PR fixes this issue by not yielding anymore between creating and awaiting the awaitable.Types of Changes
Summary by Sourcery
Fix coroutine awaiting issue in WebSocket protocol handlers by ensuring awaitables are awaited even if clients disconnect quickly.
Bug Fixes:
Documentation: