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

Replace Condvars with blocking channel #5815

Merged
merged 6 commits into from
Apr 28, 2020
Merged

Replace Condvars with blocking channel #5815

merged 6 commits into from
Apr 28, 2020

Conversation

NikVolf
Copy link
Contributor

@NikVolf NikVolf commented Apr 28, 2020

Should've been done it initially.

@NikVolf NikVolf added the A0-please_review Pull request needs code review. label Apr 28, 2020
@NikVolf NikVolf requested a review from arkpar April 28, 2020 09:40
@NikVolf NikVolf requested a review from pepyakin as a code owner April 28, 2020 09:40
let pair = Arc::new((Mutex::new(false), Condvar::new()));
let pair_clone = pair.clone();

let (sender, receiver) = std::sync::mpsc::channel();
Copy link
Member

Choose a reason for hiding this comment

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

I still don't understand why all this code isn't just:

futures::executor::block_on(futures::future::join_all(pending))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Integration tests (test_sync) are panicking, they use it and you can't have one executor inside another.

@bkchr bkchr added the B0-silent Changes should not be mentioned in any release notes label Apr 28, 2020
primitives/io/src/batch_verifier.rs Outdated Show resolved Hide resolved
@bkchr
Copy link
Member

bkchr commented Apr 28, 2020

Fails to compile

@gavofyork gavofyork added A8-mergeoncegreen and removed A0-please_review Pull request needs code review. labels Apr 28, 2020
@gavofyork gavofyork merged commit c83a60a into master Apr 28, 2020
@gavofyork gavofyork deleted the nv-remove-condvars branch April 28, 2020 15:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants