Add thread safety annotations to CTxMemPool methods

hebasto commented at 10:42 pm on August 2, 2020: member

Split out from #19306.

Only trivial thread safety annotations and lock assertions added. No new locks. No behavior change.

This is a step to make CTxMemPool::cs an instance of Mutex rather RecursiveMutex.

hebasto commented at 10:45 pm on August 2, 2020: member

@ajtowns @jnewbery @vasild @JeremyRubin

Mind reviewing?

JeremyRubin commented at 10:58 pm on August 2, 2020: contributor

this looks correct, concept and like cr ack.

DrahtBot added the label Mempool on Aug 2, 2020

DrahtBot commented at 1:50 am on August 3, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

jnewbery commented at 10:37 am on August 3, 2020: member

Concept ACK. Annotating the existing locking logic is always useful.

in src/txmempool.cpp:828 in f084aacb6e outdated

824 }
825 
826-TxMempoolInfo CTxMemPool::info(const uint256& txid) const { return info(GenTxid{false, txid}); }
827+TxMempoolInfo CTxMemPool::info(const uint256& txid) const
828+{
829+    AssertLockNotHeld(cs);

MarcoFalke commented at 11:36 am on August 3, 2020:

Excluding locks in getters makes it impossible to get more than one consistent mempool entry, as the state of the second might change after the first one has been returned.

ajtowns commented at 12:26 pm on August 3, 2020:

AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

We’ve got infoAll for a consistent look at all mempool entries (used by DumpMempool), and apparently don’t use info for consistent views otherwise, so excluding the lock seems okay to me.

hebasto commented at 12:29 pm on August 3, 2020:

To get consistent values the external locking of CTxMemPool::cs should be used.

Anyway, this PR does not change locking behavior.

hebasto commented at 1:07 pm on August 3, 2020:

AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

When a thread safety annotation is a part of function member declaration, I found it very convenient having a corresponding AssertLock{Not}Held() statement in its definition.

Also this assertion would be really helpful when CTxMemPool::cs will actually transit from RecursiveMutex to Mutex.

in src/txmempool.cpp:857 in 24668ade19 outdated

853@@ -854,7 +854,8 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
854 
855 void CTxMemPool::ApplyDelta(const uint256 hash, CAmount &nFeeDelta) const
856 {
857-    LOCK(cs);
858+    AssertLockHeld(cs);

ajtowns commented at 11:42 am on August 3, 2020:

AssertLockHeld is annotated with ASSERT_EXCLUSIVE_LOCK which disables clang’s compile time checks that the lock was acquired beforehand. Wouldn’t it be better just to rely on the compile time checks? As it stands, AssertLockHeld could hide bugs (if ApplyDelta lost its annotation, it could then be called from somewhere without the lock held and without raising errors if it called other functions expecting cs to be held, or accessed data structures guarded by cs, this would now only be found if someone ran the code with DEBUG_LOCKORDER enabled).

(Alternatively could introduce a RequireLockHeld in sync.h that does the runtime checks while expecting the compile time checks to already be satisfied; but I’m not sure there’s much benefit in that)

hebasto commented at 12:57 pm on August 3, 2020:

https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#assert-capability-and-assert-shared-capability:

These are attributes on a function or method that does a run-time test to see whether the calling thread holds the given capability.

How this could disable clang’s compile time checks?

… if ApplyDelta lost its annotation, it could then be called from somewhere without the lock held and without raising errors if it called other functions expecting cs to be held…

Why ApplyDelta could lost its annotation? Locking a recursive mutex in any site where we think it should be locked seems worse than using a non-recursive mutex with annotated functions and data members, no?

ajtowns commented at 2:10 pm on August 3, 2020:

How this could disable clang’s compile time checks?

They’re attributes for a function that does a runtime test and unwinds the stack, ie a function like f(lock) { if (!lock) throw; }, at which point it’s safe to assume that the lock is held after the function returns, so the compile time checks are no longer necessary, so disabling the compile time checks is the whole point. (That’s true for AssertLockHeldInternal when DEBUG_LOCK_ORDER is defined, but not when it’s not defined)

You can see this if you change LOCK(cs) in CTxMemPool::isSpent() to AssertLockHeld(cs). Suddenly dispite the GUARDED_BY(cs), access to mapNextTx is allowed at compile time, and you’ll only get an error at runtime, and then only if you compile with DEBUG_LOCK_ORDER.

This feature is needed by net_processing.cpp in MaybeSetPeerAsAnnouncingHeaderAndIDs where it passes a lambda to connman.ForNode where the lambda needs to know a lock is held to access lNodesAnnouncingHeaderAndIDs, but ForNode is too general to be able to assure the lambda that the lock is held.

Why ApplyDelta could lost its annotation?

Someone makes a typo; it’s not especially likely, but with a GUARDED_BY and without AssertLockHeld the missing annotation would be caught at compile time.

Locking a recursive mutex in any site where we think it should be locked seems worse than using a non-recursive mutex with annotated functions and data members, no?

No, no – your locking changes make sense, just saying that the AssertLockHeldInternal annotation isn’t helpful here. (There’s lots of other places where it’s not helpful too)

hebasto commented at 4:07 pm on August 3, 2020:

Updated.

vasild commented at 9:33 am on August 11, 2020:

Wouldn’t it be better just to rely on the compile time checks?

I find this scary. Those compile time checks do not look mature enough to me - see how the behavior changes depending on the order of the attributes: #19668 (review). Also, they do not work with gcc and even with clang produce only a compile time warnings which could be missed if -Werror is not used.

hebasto commented at 9:00 am on September 1, 2020:

AssertLockHeld is annotated with ASSERT_EXCLUSIVE_LOCK which disables clang’s compile time checks that the lock was acquired beforehand.

It is fixed in #19668.

ajtowns commented at 12:42 pm on August 3, 2020: member

Concept ACK. Shouldn’t this PR also add GUARDED_BY(cs) to all the other non-static, non-atomic data members?

hebasto commented at 1:01 pm on August 3, 2020: member

@ajtowns

Concept ACK. Shouldn’t this PR also add GUARDED_BY(cs) to all the other non-static, non-atomic data members?

This PR split out from #19306 to make it small and easy to review. Making other non-static, non-atomic data members GUARDED_BY(cs) is a design-change decision that I’d leave out of this PR scope.

practicalswift commented at 3:48 pm on August 3, 2020: contributor

Concept ACK: thanks for adding thread-safety annotations!

refactor: Add more thread safety annotations to CTxMemPool 6dbd57fca9

refactor: Make CTxMemPool::ClearPrioritisation() private 3c7d4cb528

refactor: Add negative thread safety annotations to CTxMemPool 343b93b45d

hebasto force-pushed on Aug 3, 2020

hebasto commented at 4:06 pm on August 3, 2020: member

Updated f084aacb6eb57f638d6314cf44b267b77e84c4e4 -> 343b93b45d468a4c747b0b5f7dd397aaf30081f0 (pr19647.01 -> pr19647.02, diff):

addressed @ajtowns’ comment:

AssertLockHeld is annotated with ASSERT_EXCLUSIVE_LOCK which disables clang’s compile time checks that the lock was acquired beforehand. Wouldn’t it be better just to rely on the compile time checks?

in src/txmempool.cpp:857 in 343b93b45d

853@@ -854,7 +854,6 @@ void CTxMemPool::PrioritiseTransaction(const uint256& hash, const CAmount& nFeeD
854 
855 void CTxMemPool::ApplyDelta(const uint256 hash, CAmount &nFeeDelta) const
856 {
857-    LOCK(cs);

vasild commented at 9:18 am on August 11, 2020:

Here and in CTxMemPool::ClearPrioritisation(), CTxMemPool::GetTotalTxSize() and CTxMemPool::IsUnbroadcastTx() I would strongly advise to add AssertLockHeld(cs) where LOCK(cs) is removed. And in general in every function that is annotated EXCLUSIVE_LOCKS_REQUIRED(). It would serve as documentation to the reader and also ensure that the code does not get executed without holding the mutex.

The clang compile time checks are good, but I would say not to be the sole protection - they get silenced in an unexpected way if EXCLUSIVE_LOCKS_REQUIRED() and ASSERT_EXCLUSIVE_LOCK() are both used, do not work with gcc and produce only compile warnings with clang (could be missed if -Werror is not used). I would say they are an addition, but not replacement of run-time asserts.

promag commented at 10:06 am on August 11, 2020:

6dbd57fca9d324dd468d0ee627392bd788606bd8

they get silenced in an unexpected way @vasild do you have a code sample?

I would say they are an addition

Agree.

vasild commented at 12:36 pm on August 11, 2020:

they get silenced in an unexpected way

@vasild do you have a code sample?

Yes, see #19668 (review). There are two unexpected things:

The warnings differ depending on the order of the attributes (if more than one is specified):

0void f() EXCLUSIVE_LOCKS_REQUIRED(cs) ASSERT_EXCLUSIVE_LOCK(cs);
1// vs
2void f() ASSERT_EXCLUSIVE_LOCK(cs) EXCLUSIVE_LOCKS_REQUIRED(cs);

The presence of ASSERT_EXCLUSIVE_LOCK() alters the behavior of EXCLUSIVE_LOCKS_REQUIRED().

ajtowns commented at 1:47 pm on August 11, 2020:

Annotating both ASSERT_EXCLUSIVE_LOCK and EXCLUSIVE_LOCKS_REQUIRED on the same function makes no sense. The only reason to do the ASSERT_EXCLUSIVE_LOCK is if you’re relying on the test having succeeded for execution to continue as far as later compile-time checks are concerned, but if you’ve satisfied EXCLUSIVE_LOCKS_REQUIRED already, then there’s no need to hint the later compile-time checks, they’re already satisfied.

ajtowns commented at 1:52 pm on August 11, 2020:

It was tedious to check the callers of e.g. ApplyDelta() and their callers and their callers etc until finding either LOCK(cs) or AssertLockHeld(cs) to ensure that the mutex is indeed held in all places where ApplyDelta() is called.

The whole point of the compile time annotations is to have the compiler do the work of checking that the callers have a LOCK(cs). Note that an AssertLockHeld(cs) isn’t sufficient – any time that’s called without having the lock already held would also be a bug; and until the ASSERT_EXCLUSIVE_LOCK annotation is removed from AssertLockHeld, AssertLockHeld may in fact prevent the compiler from doing adequate checking…

vasild changes_requested

vasild commented at 9:27 am on August 11, 2020: member

Looks good except the suggestion below.

It was tedious to check the callers of e.g. ApplyDelta() and their callers and their callers etc until finding either LOCK(cs) or AssertLockHeld(cs) to ensure that the mutex is indeed held in all places where ApplyDelta() is called.

in src/txmempool.h:582 in 3c7d4cb528 outdated

578@@ -579,6 +579,8 @@ class CTxMemPool
579      */
580     std::map<uint256, uint256> m_unbroadcast_txids GUARDED_BY(cs);
581 
582+    void ClearPrioritisation(const uint256 hash) EXCLUSIVE_LOCKS_REQUIRED(cs);

promag commented at 10:08 am on August 11, 2020:

3c7d4cb52869d5943c2f36b3b8a91bb60344274c

nit, could refactor to const uint256&?

promag commented at 10:18 am on August 11, 2020: member

Code review ACK 343b93b45d468a4c747b0b5f7dd397aaf30081f0.

AssertLockNotHeld is called immediately in info(gtxid) seems unnecessary to repeat it?

When a thread safety annotation is a part of function member declaration, I found it very convenient having a corresponding AssertLock{Not}Held() statement in its definition.

Also this assertion would be really helpful when CTxMemPool::cs will actually transit from RecursiveMutex to Mutex.

Let’s not make a wave of pulls adding negative thread safety annotations everywhere.

hebasto commented at 10:39 am on August 13, 2020: member

Fellow reviewers! It seems there is a consensus that adding thread safety annotations would be much safer and more convenient after AssertLockHeldInternal() improving. Therefore, begging for #19668 review at first :)

hebasto commented at 9:50 am on September 1, 2020: member

Closed in favor of #19854.

hebasto closed this on Sep 1, 2020

laanwj referenced this in commit 99a8eb6051 on Sep 4, 2020

sidhujag referenced this in commit 45b8840fd3 on Sep 9, 2020

DrahtBot locked this on Feb 15, 2022

Add thread safety annotations to CTxMemPool methods #19647

Conflicts