Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Revert "Use std::sync::Condvar (#1732)" #9392

Merged

Conversation

ordian
Copy link
Collaborator

@ordian ordian commented Aug 21, 2018

Reverts #1732. This would hopefully allow us to detect deadlocks via --feature "deadlock_detection".

parking_lot_core now uses WaitOnAddress/WakeByAddress on Windows, so I guess the reason not to use parking_lot::Condvar is no longer relevant.

@ordian ordian added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 21, 2018
@ordian ordian requested a review from arkpar August 21, 2018 14:51
@parity-cla-bot
Copy link

It looks like @ordian signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

arkpar
arkpar previously approved these changes Aug 21, 2018
@@ -202,8 +202,8 @@ struct Verification<K: Kind> {
verifying: Mutex<VecDeque<Verifying<K>>>,
verified: Mutex<VecDeque<K::Verified>>,
bad: Mutex<HashSet<H256>>,
more_to_verify: SMutex<()>,
empty: SMutex<()>,
more_to_verify: Mutex<()>,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like these locks were previously added so that they could be used with std::sync::Condvar. Previously the Condvar waited on unverified. Do you think we could remove these?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question! Did that at first, but then I saw another PR using more_to_verify, and it wasn't obvious if they can be safely removed. Anyway, it seems to work w/o them, so I'm going to push another commit with the removal.

@ordian ordian force-pushed the refactor/use-parking-lot-everywhere branch from 3c3c6f4 to 45dafaa Compare August 21, 2018 18:02
@ordian ordian dismissed stale reviews from arkpar and andresilva August 21, 2018 18:05

stale

Copy link
Collaborator

@debris debris left a comment

Choose a reason for hiding this comment

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

lgtm

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 22, 2018
Copy link
Contributor

@andresilva andresilva left a comment

Choose a reason for hiding this comment

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

LGTM

@debris debris merged commit 491ce61 into openethereum:master Aug 22, 2018
@ordian ordian deleted the refactor/use-parking-lot-everywhere branch August 23, 2018 10:59
@5chdn 5chdn added this to the 2.1 milestone Aug 23, 2018
@5chdn
Copy link
Contributor

5chdn commented Aug 23, 2018

wow, reverting a 26 months old PR 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants