Skip to content
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

Refactor WSMessage to use tagged unions #7319

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

Dreamsorcerer
Copy link
Member

This reworks WSMessage to be a union of dataclasses which can provide much better type safety using tagged unions (on the type attribute).

I need to go over a couple of details (like what should be exported and from where, imports are a little messy currently). But, my main question before I finish it off, is whether it makes sense to have .json() only on the TEXT/BINARY messages? If we do this, it becomes easy for mypy to validate the code, requiring a msg.type is WSMsgType.TEXT check before calling .json().

Fixes #7313

@Dreamsorcerer Dreamsorcerer added the backport:skip Skip backport bot label Jun 10, 2023
Copy link

codecov bot commented Oct 22, 2024

❌ 3 Tests Failed:

Tests completed Failed Passed Skipped
3 3 0 0
View the top 3 failed tests by shortest run time
 tests.test_web_websocket
Stack Traces | 0s run time
No failure message available
 tests.test_websocket_parser
Stack Traces | 0s run time
No failure message available
 tests.test_websocket_writer
Stack Traces | 0s run time
No failure message available

To view individual test run time comparison to the main branch, go to the Test Analytics Dashboard

@bdraco
Copy link
Member

bdraco commented Oct 22, 2024

Creating the dataclasses will be significantly slower than the NamedTuple, but the property accesses will be faster...probably need to benchmark it to see what the tradeoff will be

@@ -9,7 +9,8 @@
from pytest_mock import MockerFixture

from aiohttp import WSMsgType, web
from aiohttp.http import WS_CLOSED_MESSAGE, WSMessage
from aiohttp.http import WSMessageType

Check notice

Code scanning / CodeQL

Unused import Note test

Import of 'WSMessageType' is not used.
@Dreamsorcerer
Copy link
Member Author

Creating the dataclasses will be significantly slower than the NamedTuple, but the property accesses will be faster...probably need to benchmark it to see what the tradeoff will be

A quick test locally suggests that NamedTuple will also work with unions. Let me know which you want to go with and I'll update (and then try to revert the micro-optimisations that got caught in conflict resolutions).

@Dreamsorcerer
Copy link
Member Author

Also worth noting that with dataclass we get errors because DataQueue expects a Sized thing. This suggests to me that DataQueue is probably not the best solution here as there seems to be no meaning to the size when processing WS messages. Maybe an unsized queue could be used here to be a little more optimised.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip Skip backport bot
Projects
None yet
Development

Successfully merging this pull request may close these issues.

websocket: message.json() fails with a TypeError when the message exceeds the size limit
2 participants