-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Move read lock after hasHeadState #7427
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7427 +/- ##
==========================================
- Coverage 60.12% 60.07% -0.06%
==========================================
Files 419 419
Lines 30835 30231 -604
==========================================
- Hits 18540 18161 -379
+ Misses 9228 9082 -146
+ Partials 3067 2988 -79 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to solve it:
- Define exported
HasHeadState()
which will lock the mutex (basically it will be what current non-exportedhasHeadState()
is). - Define non-exported
hasHeadState()
it will be lock free version, used internally in package, where lock is already obtained.
That way, everywhere where you know that you've obtained the lock (and author of the package normally knows its contents well), you use lock-free version (if lock is already acquired). And from outside package, you are guaranteed that access is controlled by lock.
That way you don't have to worry about order of statements (first check head, then obtain locks as in the current PR), as you will simply use lock-free version, if lock is already obtained.
The only downside of that method, is exposing HasHeadState()
(not too much of an issue, but still).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- In order to avoid duplication, can we use lock-free version of func in exported one (I've added corresponding suggestion)
- Removed bunch of added blank lines (if you've added them intentionally, please, ignore suggested code changes)
Co-authored-by: Victor Farazdagi <simple.square@gmail.com>
Co-authored-by: Victor Farazdagi <simple.square@gmail.com>
Co-authored-by: Victor Farazdagi <simple.square@gmail.com>
Co-authored-by: Victor Farazdagi <simple.square@gmail.com>
This fixes a bug raised in #7423.
hasHeadState
already calls the read lock so dead lock could happen. This moved read lock to afterhasHeadState