search

cayleygraph / cayley

An open-source graph database
https://cayley.io
Apache License 2.0
14.8k stars 1.25k forks source link

memstore add rwmutex, fix multi-thread issue #954

Open RyouZhang opened 3 years ago

RyouZhang commented 3 years ago

memstore add rwmutex, fix multi-thread issue

This change is Reviewable

RyouZhang commented 3 years ago

Sorry, but I still cannot accept your change :(

  1. 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.
  2. It's still completely unclear for me how mutexes relate to protected data. For example, it's clear that valsMu is related to vals, but is it safe to hold it while you hold indexMu? 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.
  3. 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.

SVyatoslavG commented 1 year ago

What needs to be done to merge these changes? I am experiencing panics due to this bug.