Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Additional BrokerConnection locks to synchronize protocol/IFR state #1768

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

dpkp
Copy link
Owner

@dpkp dpkp commented Mar 29, 2019

@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:

>>> import dis
>>> n = {'a': 1, 'b': 2}
>>> def foo():
...     bool(n)
...
>>> dis.dis(foo)
  2           0 LOAD_GLOBAL              0 (bool)
              2 LOAD_GLOBAL              1 (n)
              4 CALL_FUNCTION            1
              6 POP_TOP
              8 LOAD_CONST               0 (None)
             10 RETURN_VALUE

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 Reviewable

@dpkp
Copy link
Owner Author

dpkp commented Mar 31, 2019

There are still some issues related to the state_change_callback that need to be cleaned up, but I think they can be dealt with in separate PRs.

@dpkp dpkp merged commit 27cd93b into master Apr 2, 2019
@dpkp dpkp deleted the kafka_conn_locking_too branch April 2, 2019 16:32
@isamaru
Copy link
Contributor

isamaru commented Apr 3, 2019

@dpkp Thanks a lot, this looks better than my attempts. Will try 1.4.6.!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants