Replace RecursiveMutex `cs_addrLocal` with Mutex, and rename it #24108

pull w0xlt wants to merge 3 commits into bitcoin:master from w0xlt:change_cs_addrLocal changing 2 files +8 −6

w0xlt commented at 9:57 AM on January 20, 2022: contributor

This PR is related to #19303 and gets rid of the RecursiveMutex cs_addrLocal.

scripted-diff: rename cs_addrLocal -> m_addr_local_mutex

-BEGIN VERIFY SCRIPT-
sed -i 's/cs_addrLocal/m_addr_local_mutex/g' -- $(git grep --files-with-matches 'cs_addrLocal')
-END VERIFY SCRIPT-

c4a31ca267

DrahtBot added the label P2P on Jan 20, 2022
shaavan approved
shaavan commented at 1:59 PM on January 20, 2022: contributor
ACK 63ca568f484a6b36b9485165c4932938f0602b36
- The new name of cs_addrLocal -> m_addr_local_mutex is an improvement.
- I checked that m_addr_local_mutex is being used at two places in the codebase. These instances have been addressed by Asserting Lock not held before locking the mutex. So I think it's safe to convert m_addr_local_mutex from RecursiveMutex to Mutex.
hebasto commented at 3:28 PM on January 20, 2022: member

Concept ACK.

Adding AssertLockNotHeld(m_addr_local_mutex) macros should be combined with adding thread safety annotations LOCKS_EXCLUDED(m_addr_local_mutex) in the src/net.h. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#threads-and-synchronization

Also, for the safe commit history, "refactor: replace RecursiveMutex m_addr_local_mutex with Mutex" commit should follow "p2p: add assertions for m_addr_local_mutex" one.
p2p: add assertions and negative TS annotations for m_addr_local_mutex 93609c1dfa
refactor: replace RecursiveMutex `m_addr_local_mutex` with Mutex dec787d8ac
w0xlt force-pushed on Jan 20, 2022
hebasto approved
hebasto commented at 9:53 PM on January 20, 2022: member

ACK dec787d8ac2e8fb42db87431dd622bf44897bc4e, I have reviewed the code and it looks OK, I agree it can be merged.
shaavan approved
shaavan commented at 11:49 AM on January 21, 2022: contributor
reACK dec787d8ac2e8fb42db87431dd622bf44897bc4e

Changes since my last review:
- Added Thread Safety annotation, LOCKS_EXCLUDED(m_addr_local_mutex), to the Get/Set AddrLocal() functions' declaration.
- Rearranged commits so that m_addr_local_mutex is converted to Mutex, after, other conditions are made right for it.
MarcoFalke merged this on Jan 24, 2022
MarcoFalke closed this on Jan 24, 2022
sidhujag referenced this in commit 180b45eb36 on Jan 28, 2022
DrahtBot locked this on Jan 24, 2023

Contributors

Labels

Linked (view graph)

#19303 Replace all of the RecursiveMutex instances with the Mutex ones