Prevent concurrent writes to a websocket connection #1656
Conversation
WalkthroughThe update involves adding thread-safety to a Go struct, Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (1)
- tools/walletextension/userconn/user_conn.go (3 hunks)
Additional comments: 3
tools/walletextension/userconn/user_conn.go (3)
6-12: The import of the
syncpackage is correct and necessary for the use ofsync.Mutex. Ensure that there are no unused imports.40-43: The addition of the
mu sync.Mutexfield to theuserConnWSstruct is appropriate for handling concurrent writes to the websocket connection. This change should prevent the race conditions that were causing the Gateway to crash.111-112: The use of
mu.Lock()anddefer mu.Unlock()is correct for ensuring that theWriteResponsemethod is safe for concurrent use by multiple goroutines. However, ensure that there are no other methods that modify theconnfield or other shared resources without proper synchronization.
| @@ -106,6 +108,9 @@ func (w *userConnWS) ReadRequest() ([]byte, error) { | |||
| } | |||
|
|
|||
| func (w *userConnWS) WriteResponse(msg []byte) error { | |||
| w.mu.Lock() | |||
| defer w.mu.Unlock() | |||
|
|
|||
| err := w.conn.WriteMessage(websocket.TextMessage, msg) | |||
| if err != nil { | |||
| if websocket.IsCloseError(err) || strings.Contains(string(msg), "EOF") { | |||
There was a problem hiding this comment.
The error handling in the WriteResponse method checks for a close error or an "EOF" in the message. However, checking for "EOF" in the message content is not a standard way to handle websocket errors and could lead to false positives if "EOF" is part of a valid message payload. This should be reviewed and potentially refactored.
Why this change is needed
Gateway crashed because of this. See logs:
https://app.datadoghq.eu/logs?query=service%3Aobscuro_gateway_sepolia_testnet%20container_id%3A6f39991112df50e334e5c2627fa104decaae3e0977db20b27ef10540a5d843f9%20&cols=host%2Cservice&event=AgAAAYv1CizXdvjiHgAAAAAAAAAYAAAAAEFZdjFDamRRQUFDZHVZQnZoUWZ5M1FBSAAAACQAAAAAMDE4YmY1MmYtNGM3NC00MDRiLWFiNGUtOGZhNDcxN2EwNzMw&index=%2A&messageDisplay=inline&refresh_mode=sliding&stream_sort=desc&view=spans&viz=stream&from_ts=1700556971076&to_ts=1700643371076&live=true
What changes were made as part of this PR
Please provide a high level list of the changes made
PR checks pre-merging
Please indicate below by ticking the checkbox that you have read and performed the required
PR checks