- Notifications
You must be signed in to change notification settings - Fork928
chore: consolidate websocketNetConn implementations#12065
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This stack of pull requests is managed by Graphite.Learn more about stacking. Join@spikecurtis and the rest of your teammates on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It'd be nice to see a test that the 4MiB payload works. Perhaps we could use functional options to adjust it if needed and then test that it works/errors with different sizes.
| @@ -50,7 +50,7 @@ func TestTailnetAPIConnector_Disconnects(t *testing.T) { | |||
| if!assert.NoError(t,err) { | |||
| return | |||
| } | |||
| ctx,nc:=websocketNetConn(r.Context(),sws,websocket.MessageBinary) | |||
| ctx,nc:=WebsocketNetConn(r.Context(),sws,websocket.MessageBinary) | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd prefer to fix this bad use ofr.Context() since someone could look at this and get the wrong idea. (Thewebsocket.Accept invalidatesr.Context() cancellation.)
| ctx,nc:=WebsocketNetConn(r.Context(),sws,websocket.MessageBinary) | |
| ctx,nc:=WebsocketNetConn(context.Background(),sws,websocket.MessageBinary) |
I know some other places are usingr.Context() viactx := r.Context() too, but this one just seems too explicitly wrong. 😄
Perhaps this is actually a case for movingwebsocket.Accept intocodersdk.WebsocketNetConn as well (or creating a unified function,codersdk.WebsocketAcceptNetConn).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The websocket.Accept invalidates r.Context() cancellation.
I don't understand what it means to "invalidate" a context cancelation.
Are you referring to the idea that once we have a websocket, passingr.Context() is a bit pointless because the context won't get canceled before the underlying TCP connection is closed? I guess that's true enough, but changing it tocontext.Background() doesn't change anything from a functional standpoint.
Are you suggesting we don't accept a context at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The context is actually not even tied to the TCP connection, it is tied to the handler which creates a scenario where it’s unlikely to ever be cancelled in a way that matters.
https://pkg.go.dev/net/http#Hijacker
Functionally, you’re right, there’s no difference but this behavior can easily trip anyone up so its usage should be discouraged.
Uh oh!
There was an error while loading.Please reload this page.
89920d9 to99b8467CompareMerge activity
|
Consolidates websocketNetConn from multiple packages in favor of a central one in codersdk