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

Support for Condition Variables #2739

Closed
kaimast opened this issue Aug 4, 2020 · 7 comments
Closed

Support for Condition Variables #2739

kaimast opened this issue Aug 4, 2020 · 7 comments
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync

Comments

@kaimast
Copy link

kaimast commented Aug 4, 2020

Is your feature request related to a problem? Please describe.
Imho, it is fairly unintuitive that there is no equivalence to std::sync::CondVar in tokio.

Describe the solution you'd like
Provide a tokio::sync::CondVar that works with tokio::sync::Mutex.

Describe alternatives you've considered
I tried using the other primitives in tokio::sync and ended up using an AtomicUsize (to track the number of waiting tasks) and a tokio::sync::Semaphore to wake them up. It works but it is kind of ugly.

Additional context
Is this something on the todo list for tokio? If not, would patches be accepted? I could probably work on this.

@kaimast kaimast added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Aug 4, 2020
@Darksonn Darksonn added M-sync Module: tokio/sync A-tokio Area: The main tokio crate and removed A-tokio Area: The main tokio crate labels Aug 5, 2020
@carllerche
Copy link
Member

Condvars don’t really fit here. Notify would be the strategy to use. Does that fit your requirement?

@carllerche carllerche added the S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author. label Oct 25, 2020
@carllerche
Copy link
Member

This is now tracked by #3066

@Darksonn Darksonn removed the S-waiting-on-author Status: awaiting some action (such as code changes) from the PR or issue author. label Oct 28, 2020
@kaimast
Copy link
Author

kaimast commented Jan 9, 2021

Correct me if I am wrong, but I do not think this is implemented. Condition variables work in conjunction with a mutex. There is no straightforward way to do this with Notifier.

There needs to be a way to pass a mutex guard to Notifier::notified. Releasing the lock before calling notified or notify_all is not an option as it leads to various race conditions...

@Darksonn
Copy link
Contributor

Darksonn commented Jan 9, 2021

I think something like this makes sense to have. Not sure how it fits in with our async mutex though.

@kaimast
Copy link
Author

kaimast commented Feb 17, 2021

I wrote something like this for my particular use case. Ideally, a similar implementation would be part of tokio or tokio-util.

use std::sync::atomic;
use tokio::sync::{Mutex, MutexGuard, Notify};

pub struct Condvar {
    notifier: Notify,
    /// Atomic here is only used to make Convdar Send and Sync
    /// Callers still need to hold the associated mutex to avoid data races
    wait_count: atomic::AtomicU32,
}

impl Condvar {
    pub fn new() -> Self {
        Self{ wait_count: atomic::AtomicU32::new(0), notifier: Notify::new() }
    }

    pub async fn wait<'a, 'b, T>(&self, lock: MutexGuard<'a, T>, mutex: &'b Mutex<T>) -> MutexGuard<'b, T> {
        self.wait_count.fetch_add(1, atomic::Ordering::SeqCst);
        drop(lock);

        self.notifier.notified().await;
        mutex.lock().await
    }

    pub fn notify_one<'a, T>(&self, lock: MutexGuard<'a, T>) {
        let count = self.wait_count.load(atomic::Ordering::SeqCst);

        if count > 0 {
            self.wait_count.store(count-1, atomic::Ordering::SeqCst);
            drop(lock);

            self.notifier.notify_one();
        }
    }

    pub fn notify_all<'a, T>(&self, lock: MutexGuard<'a, T>) {
        let count = self.wait_count.swap(0, atomic::Ordering::SeqCst);
        drop(lock);

        for _ in 0..count {
            self.notifier.notify_one();
        }
    }
}

@Darksonn
Copy link
Contributor

Darksonn commented Feb 17, 2021

Typically you would not pass the lock to the notify methods. By using some different atomic instructions, you can make remove the need for locking around them.

Feel free to open a new issue or PR if you have a proposal for adding them.

@kaimast
Copy link
Author

kaimast commented May 3, 2021

I created pull requests for my implementation: #3741 and #3742.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-sync Module: tokio/sync
Projects
None yet
Development

No branches or pull requests

3 participants