Documented locking part 1+2 #2003

1. 
   alexanderkjeldaas commented at 4:07 am on November 11, 2012: contributor

   This pull request includes needed changes to get started using locking annotations. threadsafety.h - the set of macros. sync.h - a mixin that adds annotations to the basic locks. net.h - I added annotations to functions where the set of held locks before and after the function is called is not the same.

   Reviewers: Please look carefully at the TODOs in net.h. The pre/postconditions of those functions are entirely unclear. The set of held locks depend on the value of nHeaderStart ! nHeaderStart is also involved in both != -1 and < 0 tests.

2. o Added threadsafety.h - a set of macros using the -Wthread-safety

     feature in clang.  These macros should primarily be used to
     document which locks protect a given piece of data.  Secondary it
     can be used to document the set of held and excluded locks when
     entering a function.

   c043ff79e3
3. o Added AnnotatedMixin which adds locking annotations to the mutex

     API, compatible with clang's -Wthread-safety

   05f97d1263
4. o Annotated lock-like functions in net.h.

   o Removed unused function EndMessageAbortIfEmpty

   25511af4a5
5. 
   laanwj commented at 9:00 am on November 11, 2012: member

   ACK
6. 
   sipa commented at 11:12 am on November 11, 2012: member

   Regarding Begin/End/AbortMessage, wouldn’t it be cleaner to introduce a CMessageBuilder class, which holds a scoped lock of a referenced CNode::cs_vSend, and forwards operator« to the respective vSend? For example CNode::PushMessage(pszCommand, a1) could then become simply { CMessageBuilder m(this, pszCommand); m « a1; m.Send(); }.
7. 
   jgarzik commented at 2:22 am on November 16, 2012: contributor

   How about just building unlocked, then copying the message into vSend.

   That adds a memory copy (potentially large for “block” messages), but it eliminates the locking mess, and also eliminates the whole nHeaderStart-then-back-out-if-we-abort stuff. Clean and simple, if the memory copy burden is OK.

8. 
   BitcoinPullTester commented at 10:19 am on November 23, 2012: none

   Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/25511af4a57816c4f9bb960527f090a9719c9010 for binaries and test log.
9. 
   laanwj commented at 7:44 am on December 6, 2012: member

   I like @jgarzik’s idea. Shared-nothing passing messages is a safe and elegant default, if it turns out to be a performance burden, which I don’t believe so, it can always be optimized again without the locking mess.
10. 
    alexanderkjeldaas commented at 5:22 pm on December 6, 2012: contributor

    Hi, sorry I have not had time to follow up on these locking changes, but..

    Actually locking and synchronous shared-nothing message passing are very similar from a theoretical point of view.

    To build an intuition for this, imagine that you create a thread for every set of locks that can be held in the program at the same time, by some thread. What does this mean? It means that you create a thread for every single “group” of data that is protected by a set of locks, much like in a fine-grained message-passing implementation.

    Also, for any two locks that can be held at the same time, define a lock acquisition order (so lock A must be taken before lock B, if they can both be held by the same thread at the same time).

    Now, imagine that every time you take a lock, you instead pass a message to the designated thread with a copy of “the environment” which consists of all variables visible in the lexical scope. This message is synchronous, so you wait until that other thread returns.

    Then this is equivalent to the synchronous shared nothing message passing implementation.

    Note that no other thread can legally modify “the environment” because that would require a lock, and thus execution of said modification would have to happen in a thread which is currently blocked.

    Anyways, what this means is that the process of annotating which data is protected by which locks is exactly the same information that one encodes when restructuring a program into message-passing style. However, message-passing style is a pretty heavy transformation, as it affects both performance and correctness. Lock annotation only deals with correctness.

    Also, for performance reasons, message-passing style in C++ is often not synchronous. Asynchronous message-passing can actually be a lot harder to do right than a lock-based implementation because the number of states that the program can be in is suddenly expanded over the lock-based implementation.

    So I would suggest that the bitcoin software is not rewritten to use message-passing.

    Rather, simply use lock annotations, and as a hygiene issue, limit the size of “the environment” typically by restricting the visibility of global variables.

    Also, try to get rid of TRY_LOCK.

    Alexander

    On 6 December 2012 04:44, Wladimir J. van der Laan <notifications@github.com

    wrote:

    I like @jgarzik https://github.com/jgarzik's idea. Shared-nothing passing messages is a safe and elegant default, if it turns out to be a performance burden, which I don’t believe so, it can always be optimized again without the locking mess.

    — Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2003#issuecomment-11075710.

11. 
    laanwj commented at 6:58 pm on December 6, 2012: member

    Indeed, theoretically they are equivalent (still, in practice it is harder to mess up as subtly with message passing, as you can see in one glance what is passed instead of having to spend a lot of time carefully analyzing locks).

    But I think jgarzik was talking about one specific case, building a message that’s going to go over a socket anyway to get rid of a TRY_LOCK, not rewriting the entire application.

12. 
    alexanderkjeldaas commented at 7:31 pm on December 6, 2012: contributor

    Aha. Mea culpa. That does indeed seem like a good plan.

    On 6 December 2012 15:58, Wladimir J. van der Laan <notifications@github.com

    wrote:

    Indeed, theoretically they are equivalent. But I think jgarzik was talking about one specific case, building a message that’s going to go over a socket anyway to get rid of a TRY_LOCK, not rewriting the entire application.

    — Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2003#issuecomment-11098676.

13. 
    gavinandresen commented at 5:27 pm on December 12, 2012: contributor

    Merging; I’m excited about using clang to help us make sure locking is correct and efficient.
14. gavinandresen referenced this in commit 8a7277a578 on Dec 12, 2012
15. gavinandresen merged this on Dec 12, 2012
16. gavinandresen closed this on Dec 12, 2012

    ---

17. laudney referenced this in commit a41bd74556 on Mar 19, 2014
18. HashUnlimited referenced this in commit d499405528 on Mar 21, 2018
19. KolbyML referenced this in commit 8a07799388 on Dec 5, 2020
20. DrahtBot locked this on Sep 8, 2021

Contributors
alexanderkjeldaas laanwj sipa jgarzik BitcoinPullTester gavinandresen