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 manual websocket pingloop #11765

Merged
merged 14 commits into from
Apr 13, 2022
Merged

Add manual websocket pingloop #11765

merged 14 commits into from
Apr 13, 2022

Conversation

xacrimon
Copy link
Contributor

@xacrimon xacrimon commented Apr 6, 2022

This PR readds the pre-v9 websocket pingloop. The documentation for the new websocket library used in v9 caused some confusion on my end about interactions with proxies. We need this to send more frequent ping messages manually to avoid websocket termination by proxy.

Fixes #11763

lib/web/apiserver_test.go Outdated Show resolved Hide resolved
lib/web/apiserver_test.go Outdated Show resolved Hide resolved
lib/web/apiserver_test.go Show resolved Hide resolved
lib/web/apiserver_test.go Outdated Show resolved Hide resolved
lib/web/apiserver_test.go Outdated Show resolved Hide resolved
lib/web/terminal.go Outdated Show resolved Hide resolved
lib/web/terminal.go Show resolved Hide resolved
lib/web/terminal.go Show resolved Hide resolved
lib/web/terminal.go Outdated Show resolved Hide resolved
lib/web/apiserver_test.go Show resolved Hide resolved
@xacrimon
Copy link
Contributor Author

@greedy52 friendly ping :)

Copy link
Contributor

@greedy52 greedy52 left a comment

Choose a reason for hiding this comment

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

Overall looks good to me. Just curious if setting TCP keepalive is enough for this situation or proxy is relying on ping/poing?

lib/web/apiserver_test.go Show resolved Hide resolved
lib/web/terminal.go Outdated Show resolved Hide resolved
@xacrimon
Copy link
Contributor Author

Overall looks good to me. Just curious if setting TCP keepalive is enough for this situation or proxy is relying on ping/poing?

@greedy52 Sadly not, proxies like Cloudflare care about the activity state of the WS connection on a protocol level.

@xacrimon xacrimon requested a review from greedy52 April 12, 2022 14:05
@xacrimon xacrimon enabled auto-merge (squash) April 12, 2022 14:08
@xacrimon xacrimon merged commit 5dc160d into master Apr 13, 2022
@xacrimon xacrimon deleted the joel/readd-pingloop branch April 13, 2022 15:48
xacrimon added a commit that referenced this pull request Apr 13, 2022
@xacrimon xacrimon mentioned this pull request Apr 13, 2022
@webvictim webvictim mentioned this pull request Jun 8, 2022
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.

Investigate if websocket pings are still being sent
5 participants