doc: clarify that LOCK() internally checks whether the mutex is held

vasild commented at 10:54 am on February 17, 2023: contributor

Constructs like

0AssertLockNotHeld(m);
1LOCK(m);

are equivalent to (almost, modulo some logging differences, see below)

0LOCK(m);

for non-recursive mutexes, so it is ok to omit AssertLockNotHeld() in such cases. Requests to do the former keep coming during review process. developer-notes.md explicitly states “Combine annotations in function declarations with run-time asserts in function definitions”, but that seems to be too strong or unclear. LOCK() is also a run-time assert in this case.

Also remove LocksHeld() from the public interface in sync.h since it is only used in sync.cpp.

doc: clarify that LOCK() does AssertLockNotHeld() internally

Constructs like

```cpp
AssertLockNotHeld(m);
LOCK(m);
```

are equivalent to

```cpp
LOCK(m);
```

for non-recursive mutexes, so it is ok to omit `AssertLockNotHeld()` in
such cases.

3df37e0c78

sync: unpublish LocksHeld() which is used only in sync.cpp 91d0888921

DrahtBot commented at 10:54 am on February 17, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	hebasto, achow101

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Docs on Feb 17, 2023

ajtowns commented at 11:58 am on February 17, 2023: contributor

These aren’t precisely the same (maybe they should be?). With AssertLockNotHeld you just get a report on that lock, on stderr:

0Assertion failed: lock flock held in net.cpp:1398; locks held:
1'flock' in net.cpp:1404 (in thread 'dnsseed')

with a bare LOCK() you get a less useful report on stderr (it only shows the location of the second lock attempt, not where it’s being held):

0Assertion failed: detected double lock for 'flock' in net.cpp:1399 (in thread 'dnsseed'), details in debug log.

but get more information in debug.log:

02023-02-17T11:47:18Z DOUBLE LOCK DETECTED
12023-02-17T11:47:18Z Lock order:
22023-02-17T11:47:18Z  (*) 'flock' in net.cpp:1404 (in thread 'dnsseed')
32023-02-17T11:47:18Z  (*) 'flock' in net.cpp:1399 (in thread 'dnsseed')

vasild commented at 1:43 pm on February 17, 2023: contributor

Do you think that this is a blocker for this PR?

ajtowns commented at 2:24 pm on February 17, 2023: contributor

Do you think that this is a blocker for this PR?

I was just intending to note the difference not express an opinion (which I don’t really have at this point). Would be nicer if the approach this PR recommends was clearly better in every way, though.

in doc/developer-notes.md:945 in 3df37e0c78 outdated

940@@ -941,7 +941,9 @@ Threads and synchronization
941     internal to a class (private or protected) rather than public.
942 
943   - Combine annotations in function declarations with run-time asserts in
944-    function definitions:
945+    function definitions (`AssertLockNotHeld()` can be omitted if `LOCK()` is
946+    called unconditionally after it because `LOCK()` does the same check as

hebasto commented at 5:26 pm on February 17, 2023:

… can be omitted if <condition>

It creates additional burden for reviewing if <condition> will change in the future, doesn’t it?

ajtowns commented at 5:06 pm on February 21, 2023:

If you’re moving locks around (<condition> changes), then you should be reviewing whether AssertLockNotHeld is still appropriate either way?

vasild commented at 11:47 am on February 23, 2023:

Yes, if LOCK() is moved, then it would have to be reviewed either way.

hebasto commented at 5:26 pm on February 17, 2023: member

Agree with #27116 (comment).

bitcoin deleted a comment on Feb 23, 2023

achow101 requested review from ryanofsky on Apr 25, 2023

maflcko commented at 9:46 am on August 8, 2023: member

I don’t see the point of changing this, unless you also make the stderr output the same?

vasild commented at 1:54 pm on August 16, 2023: contributor

Keep this PR as it is
Should I change LOCK() to print everything to stderr, in addition to the log? That is, print this to stderr as well:

02023-02-17T11:47:18Z DOUBLE LOCK DETECTED
12023-02-17T11:47:18Z Lock order:
22023-02-17T11:47:18Z  (*) 'flock' in net.cpp:1404 (in thread 'dnsseed')
32023-02-17T11:47:18Z  (*) 'flock' in net.cpp:1399 (in thread 'dnsseed')

Or do people prefer to write both AssertLockNotHeld() and LOCK(), like this:

0void f()
1{
2    AssertLockNotHeld(mutex);
3    LOCK(mutex);
4    ...

If “yes”, then I will close this PR.

Or call AssertLockNotHeld(mutex) from inside LOCK() if the mutex is not recursive?

ajtowns commented at 0:38 am on August 17, 2023: contributor

I lean lightly towards (1), but prefer “(0) keep this PR as is” over (2) or (3).

vasild renamed this:
~~doc: clarify that LOCK() does AssertLockNotHeld() internally~~
doc: clarify that LOCK() internally checks whether the mutex is held
on Aug 19, 2023

hebasto approved

hebasto commented at 12:43 pm on September 1, 2023: member

ACK 91d08889218e06631f43a3dab0bae576aa46e43c, I have reviewed the code and it looks OK.

I lean lightly towards (1), but prefer “(0) keep this PR as is” over (2) or (3).

My vote is for (0) or (1).

achow101 commented at 6:57 pm on October 26, 2023: member

ACK 91d08889218e06631f43a3dab0bae576aa46e43c

I think it’s fine to have people have to look at the debug.log for additional details. Running with -daemon results in stderr being lost anyways.

achow101 merged this on Oct 26, 2023

achow101 closed this on Oct 26, 2023

doc: clarify that LOCK() internally checks whether the mutex is held #27116

Code Coverage

Reviews