Fix deadloack introduced in #402#417
Merged
Merged
Conversation
This PR fixes the deadlock introduced in #402. The deadlock was occurring because the forwardWebSocketData function was holding a lock on the subscription map (in the listenWebSocket goroutine) while Unsubscribe was blocked attempting to aquire the same lock. The forwardWebSocketData function was also blocked attempting to send data on the interfaceChan channel, which had no readers in the test being run. After fixing the locking (by making sure the lock isn't held when forwardDataFunc is called), a race condition cropped up between sending on interfaceChan (in forwardWebSocketData) and closing interfaceChan in Unsubscribe. This was fixed by making the listenWebSocket goroutine the "owner" of the interfaceChan channel -- it is now the only goroutine that sends on and closes the channel. Other goroutines singal interfaceChan should be closed by setting `_hasBeenUnsubscribed` on the channel, which is protected by a short-held lock. Note that there's a similar data race involving isClosing on webSocketClient. isClosing is set when Close is called (while holding a lock) but read in listenWebSocket without a lock. A separate PR will fix this race (so the `-race` flag can be added when running tests).
dnerdy
requested review from
StevenACoffman,
benjaminjkraft and
csilvers
as code owners
csilvers
reviewed
May 20, 2026
| hasBeenUnsubscribed bool | ||
|
|
||
| // Hold when accessing _hasBeenUnsubscribed | ||
| hasBeenUnsubscribedMu sync.Mutex |
Member
There was a problem hiding this comment.
I think the copylocks (below) is a real problem -- you have to make the mutex a pointer and then hae a constructor for this struct.
Contributor
Author
There was a problem hiding this comment.
I've updated the code to use *subscription in the subscription map, which should work as well.
csilvers
reviewed
May 20, 2026
Member
csilvers
left a comment
csilvers
left a comment
There was a problem hiding this comment.
Thank you for taking this on! I'll leave it to ben to review, my head always gets fuzzy thinking about locking.
Contributor
|
Thanks a lot, @dnerdy! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR fixes the deadlock introduced in #402. The deadlock was occurring because the forwardWebSocketData function was holding a lock on the subscription map (in the listenWebSocket goroutine) while Unsubscribe was blocked attempting to aquire the same lock. The forwardWebSocketData function was also blocked attempting to send data on the interfaceChan channel, which had no readers in the test being run.
After fixing the locking (by making sure the lock isn't held when forwardDataFunc is called), a race condition cropped up between sending on interfaceChan (in forwardWebSocketData) and closing interfaceChan in Unsubscribe. This was fixed by making the listenWebSocket goroutine the "owner" of the interfaceChan channel -- it is now the only goroutine that sends on and closes the channel. Other goroutines singal interfaceChan should be closed by setting
_hasBeenUnsubscribedon the subscription, which is protected by a short-held lock.Note that there's a similar data race involving isClosing on webSocketClient. isClosing is set when Close is called (while holding a lock) but read in listenWebSocket without a lock. #418 fixes this race (so the
-raceflag can be added when running tests).I have: