Updates in synchronization mutex for Shared data #276
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add guarded by and lock required annotations
azuchi
reviewed
Jan 16, 2024
| CCriticalSection cs_totalBytesRecv; | ||
| CCriticalSection cs_totalBytesSent; | ||
| Mutex cs_totalBytesRecv; | ||
| Mutex cs_totalBytesSent; |
There was a problem hiding this comment.
In Bitcoin, RecursiveMutex is used.
There was a problem hiding this comment.
they have only one mutex. other is changed to atomic
// Network usage totals
mutable Mutex m_total_bytes_sent_mutex;
std::atomic<uint64_t> nTotalBytesRecv{0};
```
src/net.h
Outdated
| CCriticalSection cs_vRecv; | ||
| RecursiveMutex cs_vSend; | ||
| RecursiveMutex cs_hSocket; | ||
| RecursiveMutex cs_vRecv; |
| std::vector<std::string> vAddedNodes GUARDED_BY(cs_vAddedNodes); | ||
| CCriticalSection cs_vAddedNodes; | ||
| Mutex cs_vAddedNodes; |
There was a problem hiding this comment.
In Bitcoin, RecursiveMutex is used.
There was a problem hiding this comment.
this remains the same:
mutable Mutex m_added_nodes_mutex;
| bool setBannedIsDirty; | ||
| bool fAddressesInitialized; | ||
| CAddrMan addrman; | ||
| std::deque<std::string> vOneShots; | ||
| CCriticalSection cs_vOneShots; | ||
| Mutex cs_vOneShots; |
There was a problem hiding this comment.
In Bitcoin, RecursiveMutex is used.
There was a problem hiding this comment.
This is renamed. Bitcoin has reimplemented the peer management layer.
Mutex m_addr_fetches_mutex;
| @@ -530,7 +530,7 @@ struct LocalServiceInfo { | |||
| int nPort; | |||
| }; | |||
|
|
|||
| extern CCriticalSection cs_mapLocalHost; | |||
| extern Mutex cs_mapLocalHost; | |||
There was a problem hiding this comment.
In Bitcoin, RecursiveMutex is used.
There was a problem hiding this comment.
extern GlobalMutex g_maplocalhost_mutex;
you mean they say globalmutex which is actually
class GlobalMutex : public Mutex { };
cs_sendProcessing needs to be recursive. otherwise it causes deadlocks
azuchi
approved these changes
Jan 24, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changed CCriticalSections to Mutex and RecursiveMutex as in Bitcoin.
Add GUARDED_BY and EXCLUSIVE_LOCK_REQUIRED annotations to some shared data only.
Adding them to all shared data and functions caused lots of build errors on macOS. Clang supports static analysis for thread safety with option -thread-safety-analysis. But this does not seem to be strong enough to identify scope. i.e. when lock is outside a class or a function where the data is accessed. As most of our data and locks are global, and locking is in the outer scope, it causes lots of false positives. Adding more LOCKS in the same scope to pass the compilation warning causes deadlock in our tests.