Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Nov 8, 2018

The last few builds have been failing due to thread sanitizer errors.
See rust-lang/rust#54806 (comment) for why this is happening.

This PR adds a rule to the tsan suppresions list.

@@ -31,3 +31,7 @@ race:crossbeam_deque*steal
race:Backup::next_sleeper
race:Backup::set_next_sleeper
race:WorkerEntry::set_next_sleeper

# This ignores a false positive caused by `thread::park()`/`thread::unpark()`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would you know it's a false positive?

Copy link
Author

@ghost ghost Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some discussion on it here: rust-lang/rust#54806 (comment)

The PR that changed the implementation of thread::unpark() is a small change we all agreed on that it must be correct, and the only explanation is that the error is caused by tsan not understanding fences (it only understands atomic operations).

The reported data race is in Condvar::drop. We only use SeqCst operations in thread::park() and thread::unpark() so the problem must be somewhere else.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But race:pthread_cond_destroy syntax silents all tsan errors for pthread_cond_destroy, isn't it?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I'm not sure if we can suppress a more specific set of cases.

@ghost ghost mentioned this pull request Nov 8, 2018
@ghost ghost requested a review from carllerche November 9, 2018 15:08
@ghost
Copy link
Author

ghost commented Nov 9, 2018

@carllerche How does that look? Can we merge?

@carllerche carllerche merged commit 32e1caf into tokio-rs:master Nov 9, 2018
@ghost ghost deleted the fix-tsan branch November 10, 2018 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants