Additional BrokerConnection locks to synchronize protocol/IFR state #1768
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.
@isamaru I threw this together as perhaps a middle-ground between the attempts to fix protocol-out-of-sync issues discussed in #1744 .
One small detail re: python atomic operations / locking: my understanding is that a simple boolean check on a python dict is atomic and would not require additional locks. Note the single CALL_FUNCTION opcode:
Of course, the value could still change between when the dict is loaded and when it is returned, which means the return value could be stale. But the same is true if we grab the lock, check the value, and then release the lock. As soon as we've released the lock, the value may become stale before we make our next decision. Which means that for the most part, if the value is important for decisions about state, we should check the value directly while holding the lock at the decision point. Wrapping an accessor method in a lock doesn't go far enough.
This change is