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

Add configurable websocket heartbeat #826

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

scratchings
Copy link
Contributor

Without the --heartbeat option the VNC session will timeout after 1 minute of idle time. This adds --heartbeat with a default of 30 seconds, configurable with websockify_hb in the context or the environment variable WEBSOCKIFY_HB.

This implements the VNC timeout workaround mentioned in
VNC timeout

The VNC app in OSC/ondemand will need to be updated to recognise the websockify_hb context variable.

Without the --heartbeat option the VNC session will timeout after 1 minute of idle time.
This adds --heartbeat with a default of 30 seconds, configurable with websockify_hb in the context or the environment variable WEBSOCKIFY_HB
@johrstrom
Copy link
Contributor

First off, yes please and thank you! I was going to patch this for the next release, so thank you for beating me to it!

@@ -130,7 +130,8 @@ def run_script
# successful connections so that the password can be reset
def after_script
websockify_cmd = context.fetch(:websockify_cmd, "${WEBSOCKIFY_CMD:-/opt/websockify/run}").to_s

websockify_hb = context.fetch(:websockify_hb, "${WEBSOCKIFY_HB:-30}").to_s
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if this should be expanded to websockify_heartbeat_seconds just to be a little more specific? The internal variable doesn't matter as much to me, but the user facing configuration I think may need to be a little more explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine by me. I did consider this, but left it similar to the previous line where command has been abbreviated to 'cmd'

Copy link
Contributor

Choose a reason for hiding this comment

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

I was considering the same lol. cmd was before my time, but to an admin configuring it - it may be more recognizable then hb.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to go ahead and merge this now and change it later. Thanks again for the contribution!

@johrstrom johrstrom self-requested a review March 5, 2024 19:28
@johrstrom johrstrom merged commit d562cae into OSC:master Mar 5, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants