New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fNetworkActive is not protected by a lock, use an atomic #9131
Conversation
ACK 079142b |
Is this enough? Does bool actually behave differently as an atomic if we don't use the atomic-specific functions? I would think using .exchange() would be necessary to ensure the logic is itself atomic as well. |
@luke-jr Yes. An assignment to an std::atomic results in a fully sequentially serialized behaviour. You can use .store(value, memory_order_relaxed) for example if you want weaker consistency (but still atomic) behaviour. |
@theuni Can you comment here? You probably understand CConnman's threading model best. |
Sure, making it atomic should solve the problem at hand. @sipa I avoided commenting on the network active PR because IMO the model needs to change a bit to make it completely safe. I have some of those changes queued up, but I didn't want to stand in the way of the feature. For now, let's just make this atomic. Ultimately, CConnman::Start and CConnman::Stop need to be used for network activation. They were designed to function that way, it's just not complete yet. The idea is that the network threads stop when the toggle is set, which would guarantee that p2p is really down. What's missing is the ability to bring it back up with the same config. |
Slightly OT, but when is it best to use locks vs atomics? |
@rebroad Whenever you can use atomics, atomics. They are many times faster than locks. |
utACK 079142b |
utACK 079142b
This is not the first time that a question you ask is just a google search away. There is literally tons of CS literature about synchronization primitives. |
079142b fNetworkActive is not protected by a lock, use an atomic (Jonas Schnelli)
Github-Pull: bitcoin#9131 Rebased-From: 079142b
… atomic 079142b fNetworkActive is not protected by a lock, use an atomic (Jonas Schnelli)
… atomic 079142b fNetworkActive is not protected by a lock, use an atomic (Jonas Schnelli)
… atomic 079142b fNetworkActive is not protected by a lock, use an atomic (Jonas Schnelli)
Reported by @sipa: #8996 (comment)