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.

Sign up for GitHub

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

Jump to bottom

adding await ystore start method in prepare method #299

Merged
merged 1 commit into from
May 3, 2024
Merged

adding await ystore start method in prepare method #299

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jupyter_collaboration/rooms.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ async def initialize(self) -> None:
# try to apply Y updates from the YStore for this document
read_from_source = True
if self.ystore is not None:
await self.ystore.started.wait()
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jzhang20133 You didn't apply my suggestion.

Copy link
Collaborator Author

@jzhang20133 jzhang20133 May 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@davidbrochart Following your suggestion, we moved the logic here in document room. By not applying your suggestion, are you referring to this lineself.create_task(ystore.start())? I did not add ystore.start logic here because ystore.start() is also called in yroom.broadcast_update() method. And if ystore has been started and then it is started again, an exception could be thrown and the exception could kill task groups. Instead of triggering starting of ystore at multiple places and exception handling at multiple places, we choose to simply wait for the ystore to be started, which is a great idea that you have suggested.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @jzhang20133. I'll let David respond to this when he's back in office next week and we can create a follow-up PR if needed.

@davidbrochart's point below stands. If we do something different from his suggestions, we should include an explanation and wait for his response before merging.

This one is on me folks. I missed this difference in the change before approving the PR. Let's learn from this and make sure we're properly patient going forward.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Zsailer @davidbrochart I will make sure I add explanation if change is made differently from suggested and wait for @davidbrochart 's inputs and approval on the difference in the future PRs. Thank you both for your patience and help on reviewing PRs! Really appreciated!!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@davidbrochart For this specific change, please feel free to let us know your thought here about not adding this line self.create_task(ystore.start()). I can address accordingly in another PR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT this PR doesn't start the YStore, but just waits for it to be started?
I opened #302 and #303.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, it just wait for it to be started by yroom.broadcast_update() method. If yroom is started twice, an exception could be thrown and the exception could kill task groups. We need to add exception handling for ystore.start() if it get started in multiple places.

try:
await self.ystore.apply_updates(self.ydoc)
self._emit(
Expand Down
2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ classifiers = [
dependencies = [
"jupyter_server>=2.0.0,<3.0.0",
"jupyter_ydoc>=2.0.0,<3.0.0",
"pycrdt-websocket>=0.13.1,<0.14.0",
"pycrdt-websocket>=0.13.2,<0.14.0",
"jupyter_events>=0.10.0",
"jupyter_server_fileid>=0.7.0,<1",
"jsonschema>=4.18.0"
Expand Down
Loading