Avoid deadlocks by consolidating locks #412
Conversation
benjaminjkraft
requested review from
StevenACoffman,
csilvers and
dnerdy
as code owners
HaraldNordgren
left a comment
HaraldNordgren
left a comment
There was a problem hiding this comment.
Sounds reasonable. However, the actual problem has been hard for me to reproduce consistently.
I would suggest re-running all the CI 10 times, and if it passes all of those then I would consider it solved, I was never able to get 10 passes in a row (for Go 1.23, 1.24, 1.25 so 30 test runs) to pass.
Easy way to re-trigger all the CI: git commit --amend --no-edit --date="now" && git push -f
| if w.isClosing { | ||
| w.Lock() | ||
| isClosing := w.isClosing | ||
| w.Unlock() |
There was a problem hiding this comment.
Isn't it better to always follow this pattern?
w.Lock()
defer w.Unlock()
isClosing := w.isClosingThere was a problem hiding this comment.
I don't think that will work here since the lock is happening inside a loop. The defer won't run until the entire function exits, which means that the second time through the loop the Lock() would block since the lock is already being held from the first time.
You could make a separate function that just assigns w.isClosing, and use defer in that, but that's possibly overkill for this.
csilvers
left a comment
csilvers
left a comment
There was a problem hiding this comment.
I'm not really qualified to review, but I appreciate being explicit about what functions need the lock held!
| return false | ||
| } | ||
|
|
||
| // Mark as closed before actually closing to prevent double-close |
There was a problem hiding this comment.
Is this really needed given you're in a lock for this whole function? In any case, I don't think it hurts.
| w.Lock() | ||
| sub := w.subscriptions.get(wsMsg.ID) | ||
| if sub == nil { | ||
| w.Unlock() | ||
| return fmt.Errorf("received message for unknown subscription ID '%s'", wsMsg.ID) | ||
| } | ||
| if sub.hasBeenUnsubscribed { | ||
| if sub.closed { | ||
| // Already closed, ignore message | ||
| w.Unlock() | ||
| return nil | ||
| } | ||
|
|
||
| if wsMsg.Type == webSocketTypeComplete { | ||
| sub.hasBeenUnsubscribed = true | ||
| w.subscriptions.map_[wsMsg.ID] = sub | ||
| reflect.ValueOf(sub.interfaceChan).Close() | ||
| // Server is telling us the subscription is complete | ||
| w.subscriptions.closeChannel(wsMsg.ID) | ||
| w.subscriptions.delete(wsMsg.ID) | ||
| w.Unlock() | ||
| return nil | ||
| } | ||
|
|
||
| // Forward the data to the subscription channel. | ||
| // We release the lock while calling the forward function to avoid holding | ||
| // the lock while doing potentially slow user code. | ||
| w.Unlock() |
There was a problem hiding this comment.
In this case, I do agree that having to remember to unlock in all the exit paths is fragile. It may make sense to pull all this code out into its own function so you can use defer to close.
| w.Lock() | ||
| defer w.Unlock() | ||
| w.subscriptions.closeChannel(subscriptionID) | ||
| w.subscriptions.delete(subscriptionID) |
There was a problem hiding this comment.
The old Unsubscribe didn't delete, if I'm reading the code right. Is this a bugfix?
Alternate approach to fixing #402 -- I haven't fully checked if I think this is correct yet either but figured it was at least useful to put up as a sketch for discussion in #407.
TODO: changelog, do we want the integration test changes from #407 as well
I have: