Introduce YRoomFileAPI
#24
Conversation
jupyter_rtc_core/rooms/yroom.py
Outdated
| self.awareness.observe(self.send_server_awareness) | ||
| self.ydoc.observe(lambda event: self.write_sync_update(event.update)) |
There was a problem hiding this comment.
We may only want to start these observers after the YDoc content is loaded. So we would want to do something like:
async def _start_observers(self):
await self.file_api.ydoc_content_loaded
self.ydoc.observe(...)
self.awareness.observe(...)
and start this as a separate task in __init__().
There was a problem hiding this comment.
why do we need to wait for ydoc content loaded? We should add subscribers first. It is just no action or update in ydoc so no subscription is triggered. We need to make sure subscribers are there first before we make any update to ydoc. I think loading ydoc content will update ydoc and generate first few updates.
There was a problem hiding this comment.
we need a queue here self.ydoc.observe(lambda event: self.write_sync_update(event.update)) to capture those updates before the clients are added and websocket is established.
There was a problem hiding this comment.
Second thought, It might make sense to call file content load method separately and later (not in init method) once connection is established and client is added. Because once ydoc loads content, it has server updates to broadcast to all clients but the client websocket might not yet be added when we initialize YRoom. Are we going to call YRoom initialization in prepare method in YWebsocketHandler before websocket connection happens, right?
There was a problem hiding this comment.
I had posted a comment earlier but realized I was wrong. Yeah you're right, we should start the observers immediately, otherwise the new loaded content doesn't get broadcast. 😂
I was trying to stop YRoom consumers from making updates to the YDoc before the content is loaded, but as you've called out, this isn't the right way to do it. I think I'll add an async API for getting the YDoc, Awareness, and JupyterYDoc that ensures the content is loaded. Will work on this now since you're busy with another meeting.
There was a problem hiding this comment.
Because once ydoc loads content, it has server updates to broadcast to all clients but the client websocket might not yet be added when we initialize YRoom. Are we going to call YRoom initialization in prepare method in YWebsocketHandler before websocket connection happens, right?
This should be fine!
-
If clients join before the doc is loaded: they get a
SyncUpdatecontaining the new content. -
If clients join after: they get the content after completing the first client SS1 + server SS2 handshake.
There was a problem hiding this comment.
Actually, clients will always join after, since we don't process the message queue until after the ydoc content is loaded.
| def load_ydoc_content(self) -> None: | ||
| """ | ||
| Loads the file from disk asynchronously into `self.jupyter_ydoc`. | ||
| Consumers should `await file_api.ydoc_content_loaded` before performing | ||
| any operations on the YDoc. | ||
| """ | ||
| # If already loaded/loading, return immediately. | ||
| # Otherwise, set loading to `True` and start the loading task. | ||
| if self._ydoc_content_loaded.is_set() or self._ydoc_content_loading: | ||
| return | ||
| self._ydoc_content_loading = True | ||
| self._loop.create_task(self._load_ydoc_content()) |
There was a problem hiding this comment.
BTW, we can technically call this in YRoomFileAPI.__init__(), but I think as a design principle, instantiating classes shouldn't produce side-effects. Operations should be made explicit for readability.
| fileid_manager=fileid_manager, | ||
| contents_manager=contents_manager | ||
| ) | ||
| self.file_api.load_ydoc_content() |
There was a problem hiding this comment.
if load_ydoc_content() is called in this initialize method, it need to be moved to be after observer setup line 77 and 78. because creating task of ydoc means it could happen at any time for now on.
There was a problem hiding this comment.
we discussed to use a different message queue to handle awareness messages and unblock awareness message handling no matter initial file loading finished or not.
|
we discussed to use a different message queue to handle awareness messages and unblock awareness message handling no matter initial file loading finished or not. |
|
@jzhang20133 Thank you for leaving so much helpful feedback! Merging. |
2 participants
Description
Updated
YRoomto create a JupyterYDoc usingjupyter_ydoc.Introduces
YRoomFileAPI, a class that provides an API to a single file for a singleYRoomvia the Jupyter ServerContentsManager.The
YRoomFileAPIaccepts the room's ID and JupyterYDoc and loads the content whenfile_api.load_ydoc_content()is called.Provides the
file_api.ydoc_content_loadedawaitable to allow consumers to await. This will only resolve once the content is loaded into the YDoc.Provides the
schedule_save()method to allow consumers to schedule saving the YDoc to disk. Automatically waits for the content to be loaded.Added
async get_jupyter_ydoc(),async get_ydoc(), andget_awareness()methods onYRoom. Awaiting these methods ensures that the content was loaded before you receive a reference to the JupyterYDoc/YDoc.Adds a unit test that verifies
YRoomFileAPIfor plaintext files!Future work
Add a unit test that verifies
YRoomFileAPIfor notebook files.Add unit tests for updating the JupyterYDoc & saving via
schedule_save().