Skip to content

Commit

Permalink
Merge bitcoin#27116: doc: clarify that LOCK() internally checks wheth…
Browse files Browse the repository at this point in the history
…er the mutex is held

91d0888 sync: unpublish LocksHeld() which is used only in sync.cpp (Vasil Dimov)
3df37e0 doc: clarify that LOCK() does AssertLockNotHeld() internally (Vasil Dimov)

Pull request description:

  Constructs like

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

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

  ```cpp
  LOCK(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`.

ACKs for top commit:
  achow101:
    ACK 91d0888
  hebasto:
    ACK 91d0888, I have reviewed the code and it looks OK.

Tree-SHA512: c4b7ef2c0bfeb28d1c4f55f497810f629873137e02f5a92137c02cb1ff603ac76473dcd2171e594491494a5cb87b8c0c803e06b86f190d4acb231791e28e802d
  • Loading branch information
achow101 committed Oct 26, 2023
2 parents b72cb78 + 91d0888 commit e789b30
Show file tree
Hide file tree
Showing 3 changed files with 4 additions and 3 deletions.
4 changes: 3 additions & 1 deletion doc/developer-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -950,7 +950,9 @@ Threads and synchronization
internal to a class (private or protected) rather than public.
- Combine annotations in function declarations with run-time asserts in
function definitions:
function definitions (`AssertLockNotHeld()` can be omitted if `LOCK()` is
called unconditionally after it because `LOCK()` does the same check as
`AssertLockNotHeld()` internally, for non-recursive mutexes):
```C++
// txmempool.h
Expand Down
2 changes: 1 addition & 1 deletion src/sync.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ void LeaveCritical()
pop_lock();
}

std::string LocksHeld()
static std::string LocksHeld()
{
LockData& lockdata = GetLockData();
std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
Expand Down
1 change: 0 additions & 1 deletion src/sync.h
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ template <typename MutexType>
void EnterCritical(const char* pszName, const char* pszFile, int nLine, MutexType* cs, bool fTry = false);
void LeaveCritical();
void CheckLastCritical(void* cs, std::string& lockname, const char* guardname, const char* file, int line);
std::string LocksHeld();
template <typename MutexType>
void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, MutexType* cs) EXCLUSIVE_LOCKS_REQUIRED(cs);
template <typename MutexType>
Expand Down

0 comments on commit e789b30

Please sign in to comment.