Skip to content
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

Waking the other side on async consumer / producer close #30

Merged
merged 5 commits into from
Jul 29, 2023

Conversation

Congyuwang
Copy link
Contributor

No description provided.

Comment on lines +178 to +190
let t0 = std::thread::spawn(move || {
execute!(async {
drop(prod);
assert_eq!(stage.fetch_add(1, Ordering::SeqCst), 0);
});
});
let t1 = std::thread::spawn(move || {
execute!(async {
cons.wait_occupied(1).await;
assert_eq!(stage_clone.fetch_add(1, Ordering::SeqCst), 1);
assert!(cons.is_closed());
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Must separate as two polling, otherwise waking the wrong party also passes this test.

Copy link
Owner

Choose a reason for hiding this comment

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

Good note, join!-ing multiple futures can actually lead to falsely passing this test because of spurious wake-ups.

Also the usage of some async runtime (like Tokio or async-std) may be considered for testing. Each async block then can be ran in separate task.

Comment on lines +5 to +8
pub trait AsyncRingBuffer: RingBuffer + AsyncProducer + AsyncConsumer {
fn wake_producer(&self);
fn wake_consumer(&self);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add two methods for waking producer / consumer for trait AsyncRingBuffer

Copy link
Owner

Choose a reason for hiding this comment

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

I doubt that such low-level controls should be exposed to user but I can't suggest a better way to do this yet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Just realized it's interesting that Rust does not actually have private trait fn.

@agerasev
Copy link
Owner

Okay, the request looks good. I've also began to write test on this, but yours seem to be better. I'm merging it, thank you for your contribution!

@agerasev agerasev merged commit b61f51d into agerasev:master Jul 29, 2023
1 check passed
@Congyuwang Congyuwang deleted the wake-on-close branch July 29, 2023 15:04
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