jupyter-server / pycrdt-websocket

WebSocket Connector for pycrdt
https://jupyter-server.github.io/pycrdt-websocket
MIT License
14 stars 10 forks source link

fix ystore start flow #40

Closed jzhang20133 closed 6 months ago

jzhang20133 commented 6 months ago

This lineself.db_initialized = Event() in start() method replace self.db_initialized with a new Event() and when there is another call to ystore.read() or ystore.write() ahead of db initialization, this another call could be wait for the old self.db_initialized Event (created in initialize method) which is never set. In UI, file access is blocked and the loading sign keeps spinning forever.

Address this open issue: https://github.com/jupyter-server/pycrdt-websocket/issues/41

Zsailer commented 6 months ago

Nice find @jzhang20133!

I think this removes the wrong line though. I think we should remove the initialization that happens in the __init__ method instead.

jzhang20133 commented 6 months ago

In the test_version method, we want to be able to restart ystore after its db is closed. Hence we are creating a new Event() at bottom of stop method so when it is restarted, db can be initialized again.

Zsailer commented 6 months ago

@davidbrochart This PR is in response to another issue we are seeing where documents never open (i.e. spinning wheel forever).

We haven't been able to reproduce with vanilla JupyterLab + jupyter-collaboration + pycrdt-websocket yet; though, admittedly we haven't had a lot of time to try to create an example in the vanilla case.

The only thing that is different on our end is we do some authentication that could take 1-2 seconds before connecting to a room. I think we're seeing this latency cause some issues with the db_initialization.

davidbrochart commented 6 months ago

Would you mind opening an issue, that this PR references? Otherwise it gets difficult understanding what this PR is supposed to fix.

jzhang20133 commented 6 months ago

@davidbrochart I have updated the PR to follow your suggestions to remove self.db_initialized in init method. Would you like to review it one more time? cc @Zsailer

jzhang20133 commented 6 months ago

With current implementation, the room initialization method in YDocWebsocketHandler which load contents from ystore will fail directly instead of waiting for ystore to start. Due to that failure, websocket is teared down and ystore got stopped and there is no time to initialize db and file can't be opened.

[I 2024-05-02 10:31:55.199 ServerApp] Request for Y document 'TestRTC2/Untitled59.ipynb' with room ID: f851cc33-069d-4d70-9b56-6e369d48d066
[I 2024-05-02 10:31:55.581 YDocExtension] Creating FileLoader for: TestRTC2/Untitled59.ipynb
[I 2024-05-02 10:31:55.585 YDocExtension] Watching file: TestRTC2/Untitled59.ipynb
[I 2024-05-02 10:31:55.588 ServerApp] Initializing room json:notebook:f851cc33-069d-4d70-9b56-6e369d48d066
[E 2024-05-02 10:31:55.594 ServerApp] Error initializing: TestRTC2/Untitled59.ipynb
    RuntimeError('ystore is not started')
    Traceback (most recent call last):
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/jupyter_collaboration/handlers.py", line 233, in open
        await self.room.initialize()
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/jupyter_collaboration/rooms.py", line 104, in initialize
        await self.ystore.apply_updates(self.ydoc)
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/pycrdt_websocket/ystore.py", line 137, in apply_updates
        async for update, *rest in self.read():
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/pycrdt_websocket/ystore.py", line 417, in read
        raise RuntimeError("ystore is not started")
    RuntimeError: ystore is not started
[E 2024-05-02 10:31:55.599 ServerApp] Failed to write message
    Traceback (most recent call last):
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/jupyter_collaboration/handlers.py", line 266, in send
        self.write_message(message, binary=True)
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 332, in write_message
        raise WebSocketClosedError()
    tornado.websocket.WebSocketClosedError
[I 2024-05-02 10:31:55.600 ServerApp] Deleting Y document from memory: json:notebook:f851cc33-069d-4d70-9b56-6e369d48d066
[I 2024-05-02 10:31:55.600 ServerApp] Room json:notebook:f851cc33-069d-4d70-9b56-6e369d48d066 deleted
[I 2024-05-02 10:31:55.601 ServerApp] Deleting file TestRTC2/Untitled59.ipynb
[E 2024-05-02 10:31:55.674 ServerApp] Uncaught exception GET /api/collaboration/room/json:notebook:f851cc33-069d-4d70-9b56-6e369d48d066?sessionId=007deb49-2495-4202-9360-6eea43b39253 (127.0.0.1)
    HTTPServerRequest(protocol='http', host='localhost:8888', method='GET', uri='/api/collaboration/room/json:notebook:f851cc33-069d-4d70-9b56-6e369d48d066?sessionId=007deb49-2495-4202-9360-6eea43b39253', version='HTTP/1.1', remote_ip='127.0.0.1')
    Traceback (most recent call last):
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/web.py", line 1790, in _execute
        result = await result
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/jupyter_collaboration/handlers.py", line 209, in get
        return await super().get(*args, **kwargs)
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 273, in get
        await self.ws_connection.accept_connection(self)
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 863, in accept_connection
        await self._accept_connection(handler)
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 946, in _accept_connection
        await self._receive_frame_loop()
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 1105, in _receive_frame_loop
        self.handler.on_ws_connection_close(self.close_code, self.close_reason)
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 571, in on_ws_connection_close
        self.on_connection_close()
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/tornado/websocket.py", line 563, in on_connection_close
        self.on_close()
      File "/Users/jialinzhang/miniconda3/envs/jlab4-fresh/lib/python3.10/site-packages/jupyter_collaboration/handlers.py", line 333, in on_close
        if isinstance(self.room, DocumentRoom) and self.room.clients == [self]:
    AttributeError: 'YDocWebSocketHandler' object has no attribute 'room'

We would need to start ystore and then await for db_initialized in prepare method to get out of this race condition.

Zsailer commented 6 months ago

We would need to start ystore and then await for db_initialized in prepare method to get out of this race condition.

Have you verified that this works?

jzhang20133 commented 6 months ago

After adding a line await ystore.start() in YDocWebsocketHandler prepare method, file access will work. @Zsailer PR is raised here. https://github.com/jupyterlab/jupyter-collaboration/pull/299

jzhang20133 commented 6 months ago

I also have to change ystore to await _init_db method within start method to get it working. cc @Zsailer @davidbrochart

jzhang20133 commented 6 months ago

added an open issue here https://github.com/jupyter-server/pycrdt-websocket/issues/41 cc @davidbrochart @Zsailer

Zsailer commented 6 months ago

Looks good to me! Thanks @jzhang20133 for the great work!

davidbrochart commented 6 months ago

@jzhang20133 Could you send a follow-up PR with my suggested changes?

I opened #42.