-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix WebSocket compressed sends to be cancellation safe #11726
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
Conversation
|
real world performance is awful for small compressed payloads. codspeed will likely show this as well. Our current tests will actually undershow it |
|
I need to optimize it for the small payload case which is the majority of cases |
CodSpeed Performance ReportMerging #11726 will not alter performanceComparing Summary
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #11726 +/- ##
========================================
Coverage 98.73% 98.73%
========================================
Files 127 127
Lines 43546 43663 +117
Branches 2320 2325 +5
========================================
+ Hits 42996 43112 +116
Misses 390 390
- Partials 160 161 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Pull Request Overview
This PR fixes a data corruption bug in WebSocket compressed message sending when operations are cancelled. The WebSocket connection maintains a stateful compressor that can become corrupted if send_frame() is cancelled during compression, causing subsequent messages to contain mixed data.
Key changes:
- Implements shield + lock pattern for large compressed sends to ensure atomicity
- Synchronous compression path for small payloads (≤16KiB) with lock protection
- Removed lock from
ZLibCompressor.compress()and added cancellation safety warnings
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
aiohttp/_websocket/writer.py |
Refactored send_frame() to use shield+lock pattern for large compressed frames; added sync compression path for small frames |
aiohttp/compression_utils.py |
Removed internal lock from ZLibCompressor.compress(); added documentation warnings about cancellation safety |
tests/test_websocket_writer.py |
Added regression tests for single and multiple cancelled compression operations |
tests/conftest.py |
Added slow_executor fixture to simulate slow operations for testing race conditions |
CHANGES/11725.bugfix.rst |
Added changelog entry for the bug fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ge_payloads' into websocket_send_cancel_safe_large_payloads
Backport to 3.13: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 6cffcfd on top of patchback/backports/3.13/6cffcfd749734551c0c824658f3a5c9250e9a165/pr-11726 Backporting merged PR #11726 into master
🤖 @patchback |
Backport to 3.14: 💔 cherry-picking failed — conflicts found❌ Failed to cleanly apply 6cffcfd on top of patchback/backports/3.14/6cffcfd749734551c0c824658f3a5c9250e9a165/pr-11726 Backporting merged PR #11726 into master
🤖 @patchback |
(cherry picked from commit 6cffcfd)
(cherry picked from commit 6cffcfd)
(cherry picked from commit 6cffcfd)
(cherry picked from commit 6cffcfd)
What do these changes do?
Fixes data corruption in WebSocket compressed messages when send operations are cancelled. WebSocket uses a stateful compressor that persists across the connection lifetime. If a
send_frame()call is cancelled during compression, the compressor state becomes corrupted, causing subsequent messages to contain mixed/corrupted data.The fix wraps compressed sends in
asyncio.shield()+ lock pattern to ensure atomicity:HTTP doesn't need this protection because connections close on error, getting a fresh compressor for the next request.
Are there changes in behavior for the user?
No breaking changes. This is a bug fix that prevents data corruption. Users may notice:
Is it a substantial burden for the maintainers to support this?
No. The implementation is straightforward
The fix follows established patterns and includes cancellation tests for both single and multiple concurrent sends across all zlib backends.
Related issue number
Fixes #11725