cayley
cayley copied to clipboard
memstore add rwmutex, fix multi-thread issue
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. I think memory store is a special case, only for query speed, so if you want to write/read concurrently, you must make write single thread, read concurrently.
What needs to be done to merge these changes? I am experiencing panics due to this bug.