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

Add Mutex::poll_lock method #1972

Closed
canndrew opened this issue Nov 11, 2019 · 5 comments
Closed

Add Mutex::poll_lock method #1972

canndrew opened this issue Nov 11, 2019 · 5 comments
Labels
A-lock Area: futures::lock C-feature-request

Comments

@canndrew
Copy link
Contributor

I just had a bug in some code that looks like this:

use futures::lock::Mutex;

struct MySender {
    inner: Arc<Mutex<Inner>>,
}

struct MyReceiver {
    inner: Arc<Mutex<Inner>>,
}

impl Future for MyReceiver {
    fn poll(self: Pin<&mut Self>, cx: &mut Context) -> Poll<()> {
        if let Poll::Ready(inner) = Pin::new(&mut self.get_mut().inner.lock()).poll(cx) {
            // do some stuff
            return Poll::Ready(());
        }
        Poll::Pending
    }
}

The bug is that the MutexLockFuture returned by Mutex::lock deregisters itself when it gets dropped. So if I poll MyReceiver and the lock is held the task won't get woken up again when the lock is released. AFAICT my options for fixing this are:

  1. Store the MutexLockFuture inside MyReceiver. This is inconvenient and inefficient since MutexLockFuture borrows the Mutex and so I'd need to use rental and an extra layer of boxing.
  2. Implement MyReceiver with an async block/function instead of implementing Future directly. Again this means an extra layer of boxing. Also I'm trying to implement a low-level primitive here and I'd rather just implement Future directly.
  3. Use a std non-async Mutex. This is obviously less than ideal, though I know in my case the lock won't get held for very long.

At the moment I've gone for option (2) since it's the laziest option, but it would be nice if there was a simple way to do this properly.

Could we add a poll_lock method to Mutex which allows me to use the Mutex in the buggy way I attempted above?

@Nemo157
Copy link
Member

Nemo157 commented Nov 11, 2019

Presumably this would have to return some kind of token and be paired with a Mutex::{abandon/deregister/bikeshed} call in Drop::drop to allow you to deregister interest in the Mutex if your future is canceled.

You would also need to store the MutexGuard somewhere if do some stuff isn't entirely synchronous calls (which again runs into lifetime issues), or have other low-level APIs that allow you to emulate MutexGuard internally in your future (return a token proving that you have currently locked the Mutex and a function that gives you a reference to the inner value with the token, and a function that unlocks the Mutex using the token).

@canndrew
Copy link
Contributor Author

Presumably this would have to return some kind of token and be paired with a Mutex::{abandon/deregister/bikeshed} call in Drop::drop to allow you to deregister interest in the Mutex if your future is canceled.

Not really. You could just accept that the task the future was running on would get a spurious wakeup.

You would also need to store the MutexGuard somewhere if do some stuff isn't entirely synchronous calls.

Yeah, if you want to do something async in do some stuff then you're back at square one.

@Nemo157
Copy link
Member

Nemo157 commented Nov 12, 2019

I think it would be ok for a specific usecase to choose to forget the token and get a spurious wakeup, but I think the general API should support it if ergonomic.

@Matthias247
Copy link
Contributor

Not really. You could just accept that the task the future was running on would get a spurious wakeup.

It's not only that. It also means no other task will get woken up if it doesn't explicitely poll. The Mutex always only wakes up one Waker to reduce churn on the Mutex.

I implemented a solution for a similar issue in futures-intrusive for the an async Semaphore, where the SemaphoreReleaser has a disarm() method, which prevents the unlock/release on drop(). You then have to manually release it by calling semaphore.release(permits). That is an error-prone solution, but allows to store permits without the lifetime.

You could potentially use the Semaphore with a maximum amount of 1 permit in combination with an UnsafeCell and a bit of unsafe code to make it work, but it won't be pretty. In general I would recommend more to use async Mutexes as intended - inside an async fn.

@taiki-e
Copy link
Member

taiki-e commented Jun 18, 2023

I think #2571 added a reasonable alternative to this.

@taiki-e taiki-e closed this as completed Jun 18, 2023
@taiki-e taiki-e closed this as not planned Won't fix, can't repro, duplicate, stale Jun 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lock Area: futures::lock C-feature-request
Projects
None yet
Development

No branches or pull requests

4 participants