Fix deadlock on notifications #242
Merged
Conversation
This file contains 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
The notification queue is extremely vulnerable to deadlocks if one of the subscribers takes a long time to process a notification. This change makes calls to Subscribe and Publish more non-blocking by treating internal commands as an unbuffered queue, rather than a blocking channel. We also cleanup the subscriber for the response manager. Ultimately, it may make sense to move these subscribers directly into the message queue and remove the notification system entirely. but in the meantime, it makes sense for subscriber not to have access to private vars in the repsonse manager
|
should we log/warn/etc if the length of the cmds gets really big? it still seems like we'll end up at some point wanting an ability to do some sort of back-pressure, right? |
willscott
approved these changes
Oct 7, 2021
|
maybe worth a log message -- I agree. Though I don't believe this will ever get HUGE... I could be totally wrong... I dunno I think I'd prefer to make a debug message for now. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Goals
The notification queue is extremely vulnerable to deadlocks if one of the subscribers takes a long time to process a notification.
Implementation
This change makes calls to Subscribe and Publish more non-blocking by treating internal commands as an unbuffered queue, rather than a blocking channel. We use a mutex and a sync.Cond to implement, a fairly standard way to implement a non-blocking concurrent queue.
We also cleanup the subscriber for the response manager. (this was a side effect of initially trying to solve the deadlock issue by making the subscriber more non-blocking in it's processing). Ultimately, it may make sense to move these subscribers directly into the message queue and remove the notification system entirely. but in the meantime, it makes sense for subscriber not to have access to private vars in the response manager