-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
memstore add rwmutex, fix multi-thread issue #954
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks for the contribution! I appreciate your effort of solving this issue, but you'll need to add tests to prove that it's not racy. A few previous attempts to fix it failed by either racing or deadlocking.
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.
Sorry, but I still cannot accept your change :(
-
You should still add a separate test run to the CI with a
-race
flag. Without it I have no idea if the change achieves what it's supposed to. -
It's still completely unclear for me how mutexes relate to protected data. For example, it's clear that
valsMu
is related tovals
, but is it safe to hold it while you holdindexMu
? Or should one hold both while writing, or only one? Is it safe for iterator to read primitives and index, while it's modified? I don't see anything protecting*Tree
objects, for example. You should use "index hat" as I proposed, or clearly explain what can and cannot be done with those fields. -
It seems to me there quite a few holes in the current implementation, because locks just surround related fields. Usually it only masks the problem with race condition instead of fixing it. I assume currently it's possible to get multiple duplicate nodes if the code happen to write the same node concurrently, probably the same will happen to quads. Your test doesn't show that the data is consistent, only the fact that it won't crash (hopefully).
You are right, the patch make it can't crash; if you write concurrently, the data isn't consistent. But use write queue, you can get data consistent. |
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.
I need these changes urgently
What needs to be done to merge these changes? I am experiencing panics due to this bug. |
memstore add rwmutex, fix multi-thread issue
This change is