jupyterhub / jupyter-remote-desktop-proxy

Run a Linux Desktop on a JupyterHub
BSD 3-Clause "New" or "Revised" License
105 stars 101 forks source link

Retry a few times when the initial connection fails #112

Closed sunu closed 3 weeks ago

sunu commented 1 month ago

Sometimes the initial websocket connection fails. This is a workaround that tries to connect a few times with some delay in between connection attempts when a request fails.

Relates to https://github.com/jupyterhub/jupyter-remote-desktop-proxy/issues/105. Although it doesn't solve the actual underlying issue, I think it's a reasonable fix from a UX point of view.

Here's a video of the patch in action:

https://github.com/jupyterhub/jupyter-remote-desktop-proxy/assets/1142203/81c5210e-5384-45e8-9549-c72a4b484775

welcome[bot] commented 1 month ago

Thanks for submitting your first pull request! You are awesome! :hugs:
If you haven't done so already, check out Jupyter's Code of Conduct. welcome 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! :wave:
Welcome to the Jupyter community! :tada:

github-actions[bot] commented 1 month ago

Binder :point_left: Launch a binder notebook on this branch for commit 975d0843efd9ac4ebe273631ba090ab5ebfd41b7

I will automatically update this comment whenever this PR is modified

yuvipanda commented 1 month ago

To increase review capacity, I'm going to ask @batpad - have you got a moment to give this a quick review? Happy to merge if it looks good to you

sunu commented 1 month ago

Some additional notes:

I was trying to use this fix with https://github.com/sunu/jupyter-remote-qgis-proxy which starts a QGIS instance inside a tornado request handler. I noticed that that although the VNC connection is successfully established through the retry, the started QGIS instance remained in the background not attached to the VNC session.

I had to add an additional workaround in the tornado handler that checks if websockify is already running and if not starts the QGIS process with a delay after waiting for websockify to start: https://github.com/sunu/jupyter-remote-qgis-proxy/blob/bab4ce8c66ef6b3b377f32a8827f8335d75c6816/jupyter_remote_qgis_proxy/handlers.py#L17

yuvipanda commented 3 weeks ago

So, I've lots of opinions on how to make this code better, similar to what I tried in https://github.com/jupyterhub/jupyter-remote-desktop-proxy/pull/81.

However, I'm also trying to refocus on faster review and merges instead, so we become a more welcoming and vibrant project. With that in mind, my approach turns to - 'does this work? Is it better than what we have now? does it move us significantly in the wrong direction?'

The answer to all these is 'yes, yes, no' so I'm going to merge this one.

Thanks a lot @sunu!

yuvipanda commented 3 weeks ago

@sunu jupyter-remote-desktop-proxy 2.0.1 has been released with this patch in it! Welcome to your first released contribution to the jupyterhub ecosystem (I think?). Hope to see more!

sunu commented 3 weeks ago

amazing, thank you @yuvipanda!