Refactor websocket handler to protect against concurrent writes #811
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
|
||
| func (w *webSocketResponseWriter) writeMessage(b []byte) error { | ||
| w.client.send <- b | ||
| return nil // TODO(pcj): is there a need to propagate write errors? |
There was a problem hiding this comment.
I think at best we'll end up logging them, so we probably can just log it in the websocket writer.
| // Flush implements the https://golang.org/pkg/net/http/#Flusher interface. | ||
| func (w *webSocketResponseWriter) Flush() { | ||
| // no-op | ||
| } |
There was a problem hiding this comment.
Was this always a no-op, or can we coerce the websocket writer to implement this?
|
FYI #815 was just opened as an alternative fix for the underlying issue. |
|
#815 looks like a simpler change. I'll test that one out in my environment and if it seems OK, will plan to close this PR. I did make some significant inroads on a test suite for the websocket functionality; I can contribute that as a separate PR. |
|
Thanks a lot, Paul 🙏🏻, that sounds good. |
wrapper.gotowebsocket_handler.go.websocket_client.goandwebsocket_clienthub.goto manage concurrent requests.webSocketResponseWriterto send messages to the client hub rather than write to the websocket directly.Still working on this. Needs a proper test.
Closes #713