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

Start Y Websocket Server earlier and make it resilient to crashes #294

Closed
wants to merge 1 commit into from

Conversation

Zsailer
Copy link
Member

@Zsailer Zsailer commented Apr 24, 2024

This prevents the server locking we see in #290

This PR makes two critical changes:

  1. Instead of starting the y-websocket server when the first YWebsocketHandler request is made, this PR starts the server in the startup sequence of the ExtensionApp.
  2. Handle exceptions in the ywebsocket server using the exception logging offered in Add WebsocketServer exception handler jupyter-server/pycrdt-websocket#31

This depends on this PR in Jupyter Server: jupyter-server/jupyter_server#1417

@Zsailer Zsailer added the enhancement New feature or request label Apr 24, 2024
Copy link
Contributor

Binder 👈 Launch a Binder on branch Zsailer/jupyter-collaboration/resilient-y-server

Copy link
Collaborator

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Thanks @Zsailer.
I think that it would be better to have an auto_restart parameter passed to pycrdt-websocket's WebsocketServer, that would essentially do the same as here (restart on exception). That way, it would benefit to all users of the WebSocket server, not just jupyter-collaboration. What do you think?
I can open a PR there if you want.

projects/jupyter-server-ydoc/jupyter_server_ydoc/app.py Outdated Show resolved Hide resolved
@Zsailer
Copy link
Member Author

Zsailer commented Apr 25, 2024

I think that it would be better to have an auto_restart parameter passed to pycrdt-websocket's WebsocketServer

Sure! That sounds great. I'm happy to make the PR over there as well. Just let me know.

@davidbrochart
Copy link
Collaborator

I opened jupyter-server/pycrdt-websocket#31.

@Zsailer Zsailer force-pushed the resilient-y-server branch from af4431a to a893b45 Compare April 26, 2024 21:56
@Zsailer
Copy link
Member Author

Zsailer commented Apr 26, 2024

This depends on a release of pycrdt-websocket (hence why the tests are failing).

@Zsailer
Copy link
Member Author

Zsailer commented Apr 29, 2024

Closing in favor of #295

@Zsailer Zsailer closed this Apr 29, 2024
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