Skip to content

chore: Refactor accepting websocket connections to track for close #7008

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 5 commits into from
Closed

chore: Refactor accepting websocket connections to track for close #7008

wants to merge 5 commits into from

Conversation

Emyrk
Copy link
Member

@Emyrk Emyrk commented Apr 4, 2023 •
edited
Loading

This is an idea I had.

The problem is that the Coderd API struct calls Wait() on a waitgroup for all websockets to close before the Close() method returns. The Close() method calls cancel on the parent context, but this context is not actually controlling the lifecycle of the individual request for a given websocket.

So Close() does not tell a websocket to end. It was achieving this because the parent context is tied to *wsconncache.Cache which kills the tailnet connections from the parent context.

To make this work more directly, I implemented ActiveWebsockets. This struct takes the parent context. When you call Accept on this struct, it launches a go routine that will force close the websocket if the parent context is cancelled. This all happens when you call Close() on the ActiveWebsocket struct.

This also means we can pass this struct in places like WithWebsocketSupport:

coder/tailnet/derp.go

Lines 21 to 22 in bf64a43

var mu sync.Mutex
var waitGroup sync.WaitGroup

Right now we just duplicate the sync.WaitGroup logic and return a Wait() which is just kinda annoying.

This isn't 100% better, but it does handle the tracking:

coder/tailnet/derp.go

Lines 13 to 21 in 15ed467

type HandleWebsocket func(rw http.ResponseWriter, r *http.Request, options *websocket.AcceptOptions, f func(conn *websocket.Conn))
// WithWebsocketSupport returns an http.Handler that upgrades
// connections to the "derp" subprotocol to WebSockets and
// passes them to the DERP server.
// Taken from: https://github.com/tailscale/tailscale/blob/e3211ff88ba85435f70984cf67d9b353f3d650d8/cmd/derper/websocket.go#L21
// The accept function is used to accept the websocket connection and allows the caller to
// also affect the lifecycle of the websocket connection. (Eg. to close the connection on shutdown)
func WithWebsocketSupport(accept HandleWebsocket, s *derp.Server, base http.Handler) http.Handler {

Thoughts?

@Emyrk Emyrk marked this pull request as ready for review April 4, 2023 22:30
@Emyrk Emyrk mentioned this pull request Apr 5, 2023
Merged
@github-actions
Copy link

This Pull Request is becoming stale. In order to minimize WIP, prevent merge conflicts and keep the tracker readable, I'm going close to this PR in 3 days if there isn't more activity.

@github-actions github-actions bot added the stale This issue is like stale bread. label Apr 13, 2023
@Emyrk Emyrk closed this Apr 13, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Apr 13, 2023
@github-actions github-actions bot deleted the stevenmasley/websocket_tracking branch October 6, 2023 00:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@Emyrk Emyrk

Labels
stale This issue is like stale bread.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
1 participant
@Emyrk