adding exception handling for room start tasks #30

jzhang20133 · 2024-04-23T22:21:35Z

Sometimes, we have observed issues that task group in websocket_server is no longer active and user will be stuck after that and no longer able to access files and have to restart nb servers to unblock. _task_group instance variable in websocket_server will become inactive when one of its tasks of starting room or its children task group task fail with exception.

One place where an exception can occur is in the _broadcast_update method, which is a key task in children task group of _task_group in websocket_server. In this PR, the exception is handled and logged to prevent the parent task from crashing while still displaying the issue that will allow us better handle them in the future.

Resolving open source issues:
jupyterlab/jupyter-collaboration#290
jupyterlab/jupyter-collaboration#245

welcome · 2024-04-23T22:21:38Z

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

davidbrochart

Thanks @jzhang20133.
I suggest to go a step further: stop serving a client if it fails, and stop writing to the store if it fails.
I'll merge #24 first, I don't think it will affect this PR too much, but you may have to rebase.

davidbrochart · 2024-04-24T07:25:29Z

pycrdt_websocket/yroom.py

+                        self.log.debug("Writing Y update to YStore")
+                        self._task_group.start_soon(self.ystore.write, update)
+        except Exception as e:
+            self.log.error("Error broadcast updates for room:", exc_info=e)


Suggested change

self.log.error("Error broadcast updates for room:", exc_info=e)

self.log.error("Error broadcasting YRoom updates", exc_info=e)

davidbrochart · 2024-04-24T07:33:31Z

pycrdt_websocket/yroom.py

+                    for client in self.clients:
+                        self.log.debug("Sending Y update to client with endpoint: %s", client.path)
+                        message = create_update_message(update)
+                        self._task_group.start_soon(client.send, message)


Should we remove a client if we cannot send them updates? Otherwise it will likely fail on every updated.

Suggested change

for client in self.clients:

self.log.debug("Sending Y update to client with endpoint: %s", client.path)

message = create_update_message(update)

self._task_group.start_soon(client.send, message)

for client in self.clients:

try:

self.log.debug("Sending Y update to client with endpoint: %s", client.path)

message = create_update_message(update)

self._task_group.start_soon(client.send, message)

except Exception as e:

self.log.error("Error sending Y update to client with endpoint: %s", client.path, exc_info=e)

self.clients.remove(client)

For send method in YDocWebsocketHandler class, it already have a try except and will capture all exception and log debug message. https://github.com/jupyterlab/jupyter-collaboration/blob/adcde3229c986ffe498b8e6a18857195e1605ace/projects/jupyter-server-ydoc/jupyter_server_ydoc/handlers.py#L222 It is unlikely this client.send flow that raise exception.

also if just one send failure for temporary reason, we should probably not remove client. But for some specific exception like WebSocketClosedError, we should remove.

also if just one send failure for temporary reason, we should probably not remove client.

Yeah, I think we need some tolerance here. Perhaps we have a time-based tolerance before removing the client. If it's not responding for some fixed time, say 1 minute, remove it. For tornado-based servers, we should likely match the websocket ping timeout configured by jupyter_server..

But for some specific exception like WebSocketClosedError, we should remove.

As @davidbrochart already mentioned, this package is agnostic to the server framework, so we can't use this error type. However, if we add a time-based tolerance for exception, we don't have to be so specific about the exception type here.

I think I will remove that WebsocketClosedError for now.

davidbrochart · 2024-04-24T07:41:53Z

pycrdt_websocket/yroom.py

+                    if self.ystore:
+                        self.log.debug("Writing Y update to YStore")
+                        self._task_group.start_soon(self.ystore.write, update)


Should we stop writing to YStore if it fails?

Suggested change

if self.ystore:

self.log.debug("Writing Y update to YStore")

self._task_group.start_soon(self.ystore.write, update)

if self.ystore:

try:

self._task_group.start_soon(self.ystore.write, update)

self.log.debug("Writing Y update to YStore")

except Exception as e:

self.ystore = None

self.log.error("Error writing Y update to YStore", exc_info=e)

If the Update write is caused by a temporary issue and next write restores, would the missing of one Y update cause any issue for future updates or it could auto recovers? I have been purposely adding exception in ystore update flow but I did not notice any obvious issue in UI.

davidbrochart · 2024-04-24T07:45:41Z

pycrdt_websocket/yroom.py

@@ -229,7 +232,7 @@ async def serve(self, websocket: Websocket):
                            )
                            tg.start_soon(client.send, message)
            except Exception as e:
-                self.log.debug("Error serving endpoint: %s", websocket.path, exc_info=e)
+                self.log.error("Error serving endpoint: %s", websocket.path, exc_info=e)


Should we remove a client if we cannot serve them?

Suggested change

self.log.error("Error serving endpoint: %s", websocket.path, exc_info=e)

self.log.error("Error serving endpoint: %s", websocket.path, exc_info=e)

self.clients.remove(websocket)

jzhang20133 · 2024-04-26T01:21:26Z

@davidbrochart and @Zsailer would you like to review this PR again?

davidbrochart · 2024-04-26T08:12:20Z

@jzhang20133 in the future could you create a new branch from which you send a PR? I see your branch is main.

for more information, see https://pre-commit.ci

davidbrochart · 2024-04-26T08:27:21Z

I just rebased.

davidbrochart · 2024-04-26T08:28:29Z

pycrdt_websocket/yroom.py

@@ -16,6 +16,7 @@
 from anyio.abc import TaskGroup, TaskStatus
 from anyio.streams.memory import MemoryObjectReceiveStream, MemoryObjectSendStream
 from pycrdt import Doc, Subscription
+from tornado.websocket import WebSocketClosedError


pycrdt-websocket is server agnostic, Tornado cannot be used here.

Zsailer · 2024-04-26T14:22:26Z

@jzhang20133 in the future could you create a new branch from which you send a PR? I see your branch is main.

Just to explain the reasoning here, @jzhang20133. Branching helps maintainers, so they can help you rebase or push changes directly to your branch without contaminating their local main branch's history. Otherwise, it adds work for reviewers who might be reviewing multiple PRs in parallel.

davidbrochart · 2024-04-26T14:57:21Z

I'd like to align this PR with #31, so that we don't directly handle exceptions in pycrdt-websocket, but through an optional external exception handler. That would give more control to users who want to handle exceptions and not just ignore them (although they can if they use the exception_logger). What do you think?

jzhang20133 · 2024-04-26T17:02:36Z

Sounds good, I will address those. @davidbrochart @Zsailer

jzhang20133 requested a review from davidbrochart April 23, 2024 22:21

jzhang20133 requested a review from Zsailer April 23, 2024 22:21

davidbrochart requested changes Apr 24, 2024

View reviewed changes

jzhang20133 force-pushed the main branch 2 times, most recently from 64114b9 to 598301f Compare April 26, 2024 01:07

jzhang20133 requested a review from davidbrochart April 26, 2024 01:10

jzhang20133 force-pushed the main branch 2 times, most recently from b52e3c8 to b806317 Compare April 26, 2024 01:15

Jialin Zhang and others added 2 commits April 26, 2024 10:21

adding exception handling for room start tasks

1cb672a

[pre-commit.ci] auto fixes from pre-commit.com hooks

e92d8be

for more information, see https://pre-commit.ci

davidbrochart force-pushed the main branch from 12326f5 to e92d8be Compare April 26, 2024 08:21

davidbrochart requested changes Apr 26, 2024

View reviewed changes

Zsailer mentioned this pull request Apr 26, 2024

Add WebsocketServer exception handler #31

Merged

jzhang20133 closed this Apr 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding exception handling for room start tasks #30

adding exception handling for room start tasks #30

jzhang20133 commented Apr 23, 2024

welcome bot commented Apr 23, 2024

davidbrochart left a comment

davidbrochart Apr 24, 2024

davidbrochart Apr 24, 2024

jzhang20133 Apr 26, 2024

jzhang20133 Apr 26, 2024

Zsailer Apr 26, 2024

jzhang20133 Apr 26, 2024

davidbrochart Apr 24, 2024

jzhang20133 Apr 26, 2024

davidbrochart Apr 24, 2024

jzhang20133 commented Apr 26, 2024

davidbrochart commented Apr 26, 2024

davidbrochart commented Apr 26, 2024

davidbrochart Apr 26, 2024

Zsailer Apr 26, 2024

Zsailer commented Apr 26, 2024

davidbrochart commented Apr 26, 2024

jzhang20133 commented Apr 26, 2024

	self.log.error("Error broadcast updates for room:", exc_info=e)
	self.log.error("Error broadcasting YRoom updates", exc_info=e)

	self.log.error("Error serving endpoint: %s", websocket.path, exc_info=e)
	self.log.error("Error serving endpoint: %s", websocket.path, exc_info=e)
	self.clients.remove(websocket)

adding exception handling for room start tasks #30

adding exception handling for room start tasks #30

Conversation

jzhang20133 commented Apr 23, 2024

welcome bot commented Apr 23, 2024

davidbrochart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jzhang20133 commented Apr 26, 2024

davidbrochart commented Apr 26, 2024

davidbrochart commented Apr 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zsailer commented Apr 26, 2024

davidbrochart commented Apr 26, 2024

jzhang20133 commented Apr 26, 2024