- Notifications
You must be signed in to change notification settings - Fork907
chore: add support for one-way websockets to backend#16853
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
@Emyrk Just pinging you in case you didn't see this. Let me know if you're busy, and I can find someone else to review this |
@f0ssel Steven is really heads-down on Mango stuff right now. Would you be able to take over review for this? |
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.
Overall this looks great, the tests are really good coverage and the shared implementation worked out nice.
I suggested some naming changes, mainly because we don't use "callback" very much in Go and like to use "-er" words to describe capabilities.
coderd/httpapi/httpapi.go Outdated
| @@ -282,7 +285,25 @@ func WebsocketCloseSprintf(format string, vars ...any) string { | |||
| return msg | |||
| } | |||
| func ServerSentEventSender(rw http.ResponseWriter, r *http.Request) (sendEvent func(ctx context.Context, sse codersdk.ServerSentEvent) error, closed chan struct{}, err error) { | |||
| type InitializeConnectionCallback func(rw http.ResponseWriter, r *http.Request) ( | |||
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.
| typeInitializeConnectionCallbackfunc(rw http.ResponseWriter,r*http.Request) ( | |
| typeEventSenderfunc(rw http.ResponseWriter,r*http.Request) ( |
coderd/httpapi/httpapi.go Outdated
| // open a workspace in multiple tabs, the entire UI can start to lock up. | ||
| // WebSockets have no such limitation, no matter what HTTP protocol was used to | ||
| // establish the connection. | ||
| func OneWayWebSocket(rw http.ResponseWriter, r *http.Request) ( |
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.
| funcOneWayWebSocket(rw http.ResponseWriter,r*http.Request) ( | |
| funcWebSocketEventSender(rw http.ResponseWriter,r*http.Request) ( |
coderd/httpapi/httpapi.go Outdated
| type SocketError struct { | ||
| Code websocket.StatusCode | ||
| Reason string | ||
| } |
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.
This should be declared at the package level.
And just to double check, there isn't already a type like this in websocket is there?
coderd/httpapi/httpapi_test.go Outdated
| // Our WebSocket library accepts any arbitrary ResponseWriter at the type level, | ||
| // but it must also implement http.Hijack | ||
| type mockWsResponseWriter struct { | ||
| serverRecorder *httptest.ResponseRecorder | ||
| serverConn net.Conn | ||
| clientConn net.Conn | ||
| serverReadWriter *bufio.ReadWriter | ||
| } |
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.
👍
coderd/httpapi/httpapi_test.go Outdated
| return w(b) | ||
| } | ||
| func TestOneWayWebSocket(t *testing.T) { |
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.
Because we also touch the SSE Sender, could we include some basic coverage for that if there isn't already?
@f0ssel Forgot to ping you yesterday, but I applied all your feedback and added tests for the SSE code |
Closes#16777## Changes made- Added `OneWayWebSocket` utility class to help enforce one-waycommunication from the server to the client- Updated all client client code to use the new WebSocket-basedendpoints made to replace the current SSE-based endpoints- Updated WebSocket event handlers to be aware of new protocols- Refactored existing `useEffect` calls and removed some synchronizationbugs- Removed dependencies and types for dealing with SSEs- Addressed some minor Biome warnings
9bc727e intomainUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#16775
Part 1/2, with#16855 handling the frontend changes
Changes made
OneWayWebSocketfunction that establishes WebSocket connections that don't allow client-to-server communication