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

change yroom class attribute to instance attribute and stop ystore in stop method #39

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jzhang20133
Copy link
Collaborator

@jzhang20133 jzhang20133 commented Apr 30, 2024

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.

@jzhang20133 jzhang20133 added the enhancement New feature or request label Apr 30, 2024
Comment on lines 85 to 86
self._update_send_stream, self._update_receive_stream = create_memory_object_stream(
max_buffer_size=65536
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self._update_send_stream, self._update_receive_stream = create_memory_object_stream(
max_buffer_size=65536
)

@jzhang20133 jzhang20133 force-pushed the yroom-exception branch 3 times, most recently from 794b98d to a993c27 Compare May 3, 2024 22:15
@jzhang20133 jzhang20133 changed the title change yroom class attribute to instance attribute change yroom class attribute to instance attribute and stop ystore in stop method May 3, 2024
@jzhang20133 jzhang20133 requested a review from davidbrochart May 3, 2024 22:17
@jzhang20133 jzhang20133 force-pushed the yroom-exception branch 2 times, most recently from 29dc715 to 44b8f55 Compare May 3, 2024 22:49
try:
await self.ystore.stop()
except RuntimeError:
pass
Copy link
Collaborator

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.

Copy link
Collaborator Author

@jzhang20133 jzhang20133 May 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants