fNetworkActive is not protected by a lock, use an atomic #9131

Merged
merged 1 commit into from Nov 16, 2016

Projects

None yet

7 participants

@jonasschnelli
Member

Reported by @sipa: #8996 (comment)

@jonasschnelli jonasschnelli fNetworkActive is not protected by a lock, use an atomic 079142b
@jonasschnelli jonasschnelli added the P2P label Nov 11, 2016
@paveljanik
Contributor

ACK 079142b

@luke-jr
Member
luke-jr commented Nov 12, 2016

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.

@sipa
Member
sipa commented Nov 12, 2016

@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.

@sipa
Member
sipa commented Nov 12, 2016

@theuni Can you comment here? You probably understand CConnman's threading model best.

@theuni
Member
theuni commented Nov 12, 2016

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.

@rebroad
Contributor
rebroad commented Nov 13, 2016

Slightly OT, but when is it best to use locks vs atomics?

@sipa
Member
sipa commented Nov 13, 2016

@rebroad Whenever you can use atomics, atomics. They are many times faster than locks.

@sipa
Member
sipa commented Nov 15, 2016

utACK 079142b

@laanwj
Member
laanwj commented Nov 16, 2016

utACK 079142b

Slightly OT, but when is it best to use locks vs atomics?

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.
In any case atomic access 'protects' only one variable, whereas locks can protect multiple from concurrent access. As there is nothing else that needs to remain internally consistent at the same time as fNetworkActive changes, an atomic will do.

@laanwj laanwj merged commit 079142b into bitcoin:master Nov 16, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj laanwj added a commit that referenced this pull request Nov 16, 2016
@laanwj laanwj Merge #9131: fNetworkActive is not protected by a lock, use an atomic
079142b fNetworkActive is not protected by a lock, use an atomic (Jonas Schnelli)
62af164
@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr fNetworkActive is not protected by a lock, use an atomic
Github-Pull: #9131
Rebased-From: 079142b
ea2d47c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment