bitcoin/bitcoin — pull request #1995 by alexanderkjeldaas (2012-11-09 18:37:41+00:00)
Title: Large locking change

A while back I started looking at bitcoin, and I was disappointed with the code quality wrt locking.

I tried to fix the code to be clang -Wthread-safety clean, but I don't know the code enough to understand some of the entanglements that are currently present.

I've added lots of annotations that specifies the locking requirements on functions and data. This might seem a little overzealous, but I believe that this is necessary to "get back on track" and make the locking strategy clear and consistent.

Even with this patch there are lots of locking issues left - probably benign like things happening during startup and shutdown. There are also false positives from me not getting clang to grok TRY_LOCK correctly.

Below is a long note I wrote while looking through the source code a while back. Maybe you guys agree or maybe not - at least it explains the whats and the whys.

---

For an object that is accessed by multiple threads we have, for every field:
- It is const, constant after initialization, or guarded by a mutex.

- Re-using an object is a *bad idea*. It should never be done. If we
do, we are mutating the constant-after-initialization fields. This
leads to exessive locking and overcomplicated code. It is a lot
simpler to discard the object and re-create it. This works, because
the object is owned by some other object, and in that other object,
it is a "guarded by mutex" field. Thus by re-creating the object,
we lift the responsibility for locking into the parent.

- A result of the above is that "constructors" must be separate from
other functions in the class. Constructors that are not strictly
C++ constructors, such as deserializing should be static functions
if possible. This makes it impossible for them to to be used to
mutate constant-after-initialization fields.

- Locks should not be exported outside of a class. Java did this
mistake and it is considered an anti-pattern.

- Locks should never serialize code, they should protect data from
being accessed while invariants on that data are broken.

- Locks should be textually close to the fields. Given a lock, it
should be easy to understand what it guards, and given a field, it
should be easy to see which locks guard that field.
Thus, locks should be private members of a class.

A good way to obfuscate this is to put a lock in a parent class, as
done in keystore.h where CKeyStore. It is hard to know what that
lock guards. It is a lot cleaner to duplicate that lock in the data
section of the subclasses.

- Recursive locks should never be used. They lead to bugs, and are
*only* necessary in order to write sloppy code. If some functionality
is both a transaction that ensures that the invariants of the data are
obeyed, and part of larger transactions, then it is much clearer to
separate these conserns into two functions:

void DoFoo() {
LOCK(cs);
DoFooUnlocked();
}
void DoFooUnlocked() { ... }

- Using locks to protect simple setters and getters is often buggy
and/or complex. In most cases, mutating some state is part of a
larger transaction, so the setter is often protected by some other
lock external to the class. In these cases, re-building a new
object is usually the best approach, but otherwise, explicitly
marking setters as SetFooUnlocked() and not lock is clearer. It
makes it explicit that you can't just mutate randomly without doing
some external locking.

- Do not hold locks over callbacks out of an object. This means that
objects should *signal* some state change, and not guarantee which
state the object is in when a callback happens. Sometimes, it makes
sense to break this, but then the callback will have to be treated
like a signal handler, or irq handler - i.e. it can only do simple
things and not call out to other random code.

- Every time you call TRY_LOCK, a kitten dies. The reason why this is
bad is that it makes reasoning, and testing the code incredibly
hard. TRY_LOCK is basically this:

if (rand() % 1) {
DoA();
} else {
DoB();
}

It is simply unnecessary randomness. The TRY_LOCK macro should be
removed, it kills kittens.

If something like TRY_LOCK is truly needed, then at least it should
be abstracted out of the actual functions, so that the functions
themselves are testable. Thus in the above case, DoA() would be a
separate, testable, function.

In some really large systems (millions of large tps), sharding work
queues by lock is a reasonable design to reduce resource usage by
threads (so you do cs_someLock.DoWorkNowOrWhenUnlocked(&DoA), and it
is more efficient than TRY_LOCK, but for bitcoin - this is just
overkill.

- Having lots of semantically different locks in a class is not ideal.
For example CNode has 4 locks. This means that there are 4 sets of
invariants that can be established independently of each other in
this class. To put it another way, when you call a function on this
object, it should work on a subset of the state, or it should hold
all 4 locks. This suggests that either the class is a composite of
multiple objects, and should be broken up, or the number of locks
should be reduced.

- We cannot compose multiple critical sections which all take a lock
into a large critical section. After we have released locks, all
checks that we have done are invalidated. Most of CWallet is just
plain wrong and based on this false assumption. Locking is done in
CCryptoKeyStore instead of in CWallet. See above "Locks should be
textually close to the fields" for a reason why people confuse
themselves.

- There is a lot of convoluted code like this:
bool result;
{
LOCK(cs_KeyStore);
result = (mapKeys.count(address) > 0);
}
return result;

which is equivalent to this:

LOCK(cs_KeyStore);
return (mapKeys.count(address) > 0);

The destructor is guaranteed to run after the expression is
evaluated. There is no need to over-engineer.

Similarly, there is a lot of code like this:

void SomeFunction(...)
{
something();
{
LOCK(cs_something);
...
}
}

or this:
void SomeFunction(...)
{
{
LOCK(cs_something);
.. rest of function ...
}
return false;
}

There is no point in all the excessive blocks. What are they for?