Replace RecursiveMutex cs_mapLocalHost with Mutex, and rename it
#24099
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Approach ACK 3e05ab9
Going commit wise:
- The renaming of the mutex
cs_mapLocalHost->map_localhost_mutex, is an improvement, in my opinion. - The type of
map_localhost_mutexis changed from RecursiveMutex to Mutex. I checked through the codebase wheremap_localhost_mutexis used for any instances of recursive lockings, and I found none. Also, I was able to compile this PR without any UB warnings successfully. So I think it’s safe to convert the type of this mutex from RecursiveMutex to Mutex.
I shall ACK this PR as soon as this suggestion by @hebasto is addressed.
-BEGIN VERIFY SCRIPT-
s() { sed -i 's/cs_mapLocalHost/g_maplocalhost_mutex/g' $1; }
s src/net.cpp
s src/net.h
s src/rpc/net.cpp
s src/test/net_tests.cpp
-END VERIFY SCRIPT-
3e05ab9 to
5e7e4c9
Compare
There was a problem hiding this comment.
ACK 5e7e4c9, I have reviewed the code and it looks OK, I agree it can be merged.
In particular, I've verified that there are no recursive mutex locking on the path GetLocal() --> CNetAddr::GetReachabilityFrom() --> CNetAddr::IsRoutable().
A script in the "scripted-diff" commit could be more robust:
sed -i 's/cs_mapLocalHost/g_maplocalhost_mutex/g' -- $(git grep --files-with-matches 'cs_mapLocalHost')
This PR is related to #19303 and gets rid of the
RecursiveMutex cs_mapLocalHost.