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

Retry a few times when the initial connection fails #112

Merged
merged 2 commits into from
Jun 12, 2024

Conversation

sunu
Copy link
Contributor

@sunu sunu commented May 31, 2024

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 #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:

desktop-proxy-retry.mp4

Sometimes the initial websocket connection fails. This is a workaround that retries the connection a few times with some delay in between connection attempts.
Copy link

welcome bot commented May 31, 2024

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

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! 👋

Welcome to the Jupyter community! 🎉

Copy link

Binder 👈 Launch a binder notebook on this branch for commit 975d084

I will automatically update this comment whenever this PR is modified

@yuvipanda
Copy link
Contributor

yuvipanda commented Jun 1, 2024

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
Copy link
Contributor Author

sunu commented Jun 5, 2024

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
Copy link
Contributor

So, I've lots of opinions on how to make this code better, similar to what I tried in #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 yuvipanda merged commit bec995c into jupyterhub:main Jun 12, 2024
7 checks passed
@yuvipanda yuvipanda added the bug Something isn't working label Jun 13, 2024
@yuvipanda
Copy link
Contributor

@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
Copy link
Contributor Author

sunu commented Jun 14, 2024

amazing, thank you @yuvipanda!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants