-
Notifications
You must be signed in to change notification settings - Fork 11
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
change yroom class attribute to instance attribute and stop ystore in stop method #39
base: main
Are you sure you want to change the base?
Conversation
c1fe260
to
88d0cf7
Compare
pycrdt_websocket/yroom.py
Outdated
self._update_send_stream, self._update_receive_stream = create_memory_object_stream( | ||
max_buffer_size=65536 | ||
) |
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.
self._update_send_stream, self._update_receive_stream = create_memory_object_stream( | |
max_buffer_size=65536 | |
) |
794b98d
to
a993c27
Compare
15e6901
to
a35810d
Compare
29dc715
to
44b8f55
Compare
d525b5e
to
a803af3
Compare
try: | ||
await self.ystore.stop() | ||
except RuntimeError: | ||
pass |
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.
I'm not sure we should automatically stop the YStore when the YRoom is stopped.
WebsocketServer
has an auto_clean_room
parameter, maybe there should be an auto_stop_store
as well, that we would use to do so if set to True
.
Also, we should have YStore use the same exception handler pattern that we used for YRoom
and WebsocketServer
, and not catch exceptions here.
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.
I think it is a good idea to make it configurable. And also +1 for adding exception handler pattern in other logic to protect task group. I will address those.
In this stop method case, we are trying to except a RuntimeError that is thrown if "YStore not running" in cases like ystore is already stopped or ystore is not started yet but room crashed. I can create a specific exception type for this case.
Previously, some same named instance attributes are created or defined outside of initializer and YRoom does not need class attributes hence we change yroom class attributes to instance attribute just to improve readability. In this PR we also stop ystore in yroom.stop() method.
Added a unit test to ystore stop inside yroom.stop() and unit test need #42 to be merged. Need to wait until #42 is merged and then rebased.