-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 improperly closed WebSocket connections generating a backtrace #9883
Conversation
We should only be generating a backtrace if loop.debug() is true when these get cleaned up by `__del__` in `ClientResponse`
CodSpeed Performance ReportMerging #9883 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #9883 +/- ##
=======================================
Coverage 98.71% 98.71%
=======================================
Files 118 118
Lines 36309 36347 +38
Branches 4316 4317 +1
=======================================
+ Hits 35841 35879 +38
Misses 315 315
Partials 153 153
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
The test harness is a bit too helpful as it tracks all the websocket connections and cleanups up the improperly closed ones |
Backport to 3.11: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply a118114 on top of patchback/backports/3.11/a11811471ded1a4df59302316d12fc90a3cfc819/pr-9883 Backporting merged PR #9883 into master
🤖 @patchback |
…nections generating a backtrace (#9884)
Backport to 3.12: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply a118114 on top of patchback/backports/3.12/a11811471ded1a4df59302316d12fc90a3cfc819/pr-9883 Backporting merged PR #9883 into master
🤖 @patchback |
…nections generating a backtrace (#9887)
We should only be generating a backtrace if
loop.get_debug()
is enabled when these get cleaned up by__del__
inClientResponse
. Because theis_eof
method wasn't present onWebSocketDataQueue
it would checkprotocol.should_close
viaConnection.release()
which wasn't there and generate a backtrace.This is likely happening in #9880 as a result of cancellation.
The test harness was a bit too helpful in this case as it automatically cleanups unclosed WebSocket connections which meant the missing
is_eof
method went unnoticed.fixes #9880