Fix data races in the connection state changes tests #7871
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
tgoyne
added
no-jira-ticket
Pull Request Test Coverage Report for Build thomas.goyne_454Details
💛 - Coveralls |
The assignment to token1 on the main thread and the read of it on the sync worker thread didn't have any intervening synchronization and in theory the callback could read it before it's actually written. Fixing this requires adding some locking. Conversely, listener2 doesn't actually need to be atomic since the second callback should only ever be invokved synchronously inside log_out(), and if it's called at some other time that's a bug. It doesn't matter here, but `listener1_call_cnt = listener1_call_cnt + 1` is a nonatomic increment that will drop updates if it happens on multiple threads at once, while `++` will not.
Merged
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
This produced a tsan error for me when running locally. The assignment to token1 on the main thread and the read of it on the sync worker thread didn't have any intervening synchronization and in theory the callback could read it before it's actually written. Fixing this requires adding some locking.
Conversely, listener2 doesn't actually need to be atomic since the second callback should only ever be invokved synchronously inside log_out(), and if it's called on some other thread that's a bug.
It doesn't matter here, but
listener1_call_cnt = listener1_call_cnt + 1is a nonatomic increment that will drop updates if it happens on multiple threads at once, while++will not.