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

Switch synchronization primitive usage to parking_lot #6564

Merged
merged 6 commits into from Sep 27, 2018

Conversation

Projects
None yet
3 participants
@stuhood
Copy link
Member

stuhood commented Sep 27, 2018

Problem

As described in https://internals.rust-lang.org/t/standard-library-synchronization-primitives-and-undefined-behavior/8439 , the behaviour of rust's stdlib synchronization primitives can vary on different platforms, and we suspect that this could be making some synchronization issues harder to reproduce and track down.

Solution

On a whim (and because parking_lot is very well respected), I decided to try porting pants to parking_lot::{Mutex, RwLock}, and received confirmation that a hang that some users were experiencing on Arch and Gentoo is resolved by this change.

Result

Fixes #6510, and hopefully makes the behaviour of locking more consistent cross-platform. Performance remains roughly equivalent.

@stuhood stuhood requested review from illicitonion and cosmicexplorer Sep 27, 2018

@stuhood stuhood added this to the 1.10.x milestone Sep 27, 2018

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor

cosmicexplorer commented Sep 27, 2018

Works on my machine for the repro case described in #6510!

@illicitonion
Copy link
Contributor

illicitonion left a comment

Looks great, thanks!

May be worth adding a check for grep use std::sync.*(Mutex|RWLock) or similar in build-support/bin/check_banned_imports.sh

@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Sep 27, 2018

May be worth adding a check for grep use std::sync.*(Mutex|RWLock) or similar in build-support/bin/check_banned_imports.sh

Good idea... will open a separate PR for that in order to get this fix in a bit sooner.

@stuhood stuhood merged commit b0b8b3c into pantsbuild:master Sep 27, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stuhood stuhood deleted the twitter:stuhood/parking-lot branch Sep 27, 2018

stuhood added a commit that referenced this pull request Sep 27, 2018

Switch synchronization primitive usage to parking_lot (#6564)
As described in https://internals.rust-lang.org/t/standard-library-synchronization-primitives-and-undefined-behavior/8439 , the behaviour of rust's stdlib synchronization primitives can vary on different platforms, and we suspect that this could be making some synchronization issues harder to reproduce and track down.

On a whim (and because `parking_lot` is very well respected), I decided to try porting pants to `parking_lot::{Mutex, RwLock}`, and received confirmation that a hang that some users were experiencing on Arch and Gentoo is resolved by this change.

Fixes #6510, and hopefully makes the behaviour of locking more consistent cross-platform. Performance remains roughly equivalent.

stuhood added a commit that referenced this pull request Sep 28, 2018

Add forbidden imports check to ban std::sync primitives. (#6566)
### Problem

#6564 replaces usage of `std::sync` primitives, but doesn't ban them.

### Solution

Ban them as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment