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

Doc of mutex advising std blocking mutex look totally misleading #5024

Open
Stargateur opened this issue Sep 17, 2022 · 9 comments · May be fixed by #6819
Open

Doc of mutex advising std blocking mutex look totally misleading #5024

Stargateur opened this issue Sep 17, 2022 · 9 comments · May be fixed by #6819
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-sync Module: tokio/sync T-docs Topic: documentation

Comments

@Stargateur
Copy link

Stargateur commented Sep 17, 2022

Doc of mutex currently state:

Contrary to popular belief, it is ok and often preferred to use the ordinary Mutex from the standard library in asynchronous code.

That contrary to all my understanding and all the doc of async I know. Tokio doc is very clear about a task should not block. Waiting on a std mutex is blocking, I don't see how the doc can suggest to use std mutex in async context. At the very least it should state that the user should use block_in_place

What is the rational that it's ok to block a async task but only for mutex of std ?

@Stargateur Stargateur added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Sep 17, 2022
@Stargateur Stargateur changed the title Doc of mutex advicing std blocking mutex look totally misleading Doc of mutex advising std blocking mutex look totally misleading Sep 17, 2022
@Darksonn Darksonn added T-docs Topic: documentation M-sync Module: tokio/sync labels Sep 17, 2022
@Darksonn
Copy link
Contributor

Whether or not something counts as blocking has to do with the duration it takes. Usually, you will be blocking on the mutex for a sufficiently short time that you avoid problems.

@Stargateur
Copy link
Author

Stargateur commented Sep 17, 2022

Whether or not something counts as blocking has to do with the duration it takes.

First time I read this, thus that mostly true in practice in theory that false, I think that asking for trouble. I think use mutex (as a user) in async is often a mistake, you better use channel and co that will do the unsafe job for you.

I have trouble to understand exactly the doc:

The feature that the async mutex offers over the blocking mutex is the ability to keep it locked across an .await point.

Note that, although the compiler will not prevent the std Mutex from holding its guard across .await points in situations where the task is not movable between threads, this virtually never leads to correct concurrent code in practice as it can easily lead to deadlocks.

The doc mix std and tokio mutex implementation is hard to follow, and I don't understand how tokio mutex would not deadlock like std mutex since the doc say "keep it locked across an .await point." for both mutex.

It's look like the doc should be separate in 3 sections:

  • don't use this
  • how to use std mutex in async
  • how to use tokio mutex

BTW: should the same advice of using RwLock from std instead of tokio RwLock apply ? The doc of RwLock currently doesn't at all suggest use std RwLock.

@Darksonn
Copy link
Contributor

I'm sure this particular section in the documentation could be improved, and I have a draft for a article that discusses how the std mutex is used in more detail, but I haven't had the time to work on it for a while.

The Tokio mutex doesn't deadlock when held across an .await because the lock method uses an .await, which allows the task to yield to the runtime while waiting for the mutex to become available.

Yes, it also applies to RwLock.

@jobafr
Copy link

jobafr commented Apr 24, 2024

The feature that the async mutex offers over the blocking mutex is the ability to keep it locked across an .await point. […] Note that, although the compiler will not prevent the std Mutex from holding its guard across .await points in situations where the task is not movable between threads, this virtually never leads to correct concurrent code in practice as it can easily lead to deadlocks.

As I understand it, the first sentence is a direct contradiction of the rest. I think it's also incorrect, because this works fine:

#[tokio::main]
async fn main() {
    let m = std::sync::Mutex::new(42usize);
    let l = m.lock().unwrap();
    tokio::time::sleep(core::time::Duration::from_secs(1)).await;
    drop(l);
    println!("Hello, world!");
}

The rest of the explanation is really vague. An example for why and how "this virtually never leads to correct concurrent code in practice", would be great.

@Darksonn
Copy link
Contributor

Darksonn commented Apr 24, 2024

The code you've shared is fine because there's only one user of the mutex. But if you had two async tasks running at the same time executing that code on the same mutex, then it would deadlock. (Or at least, it is very likely to.)

@Darksonn
Copy link
Contributor

To make my point clearer: Yes, there exist extremely simple programs where holding an std mutex locked over an await does not deadlock. Typically that is because you never have two tasks call lock at the same time. But such cases are very rare in real-world programs, and in essentially all real-world situations, holding an std mutex locked over an await is incorrect.

@jobafr
Copy link

jobafr commented Apr 24, 2024

Ok, thank you! I managed to come up with a simple example that breaks with std's Mutex and works with tokio's.

This one uses std::sync::Mutex and blocks forever

use std::sync::Mutex;

#[tokio::main]
async fn main() {
    let m = Box::leak(Box::new(Mutex::new(1i32)));
    let t1 = task_1(m);
    let t2 = task_2(m);
    tokio::join!(t1, t2);
}


async fn task_1 (m : &Mutex<i32>) {
    let l = m.lock().unwrap();
    tokio::time::sleep(core::time::Duration::from_secs(2)).await;
    drop(l);
    println!("task_1 done");
}

async fn task_2 (m : &Mutex<i32>) {
    tokio::time::sleep(core::time::Duration::from_secs(2)).await;
    let _unused = m.lock().unwrap(); // this will never return, because the runtime is stuck here instead of returning to task_1's sleep await point
    drop(_unused);
    println!("task_1 done");
}

This one uses tokio's and works, because lock() in task_2 does not block the scheduler and tokio can continue running task_1. Therefore, task_1 can actually cross its await point and release the lock, allowing task_2 to run.

use tokio::sync::Mutex;

#[tokio::main]
async fn main() {
    let m = Box::leak(Box::new(Mutex::new(1i32)));
    let t1 = task_1(m);
    let t2 = task_2(m);
    tokio::join!(t1, t2);
}


async fn task_1 (m : &Mutex<i32>) {
    let l = m.lock().await;
    tokio::time::sleep(core::time::Duration::from_secs(2)).await;
    drop(l);
    println!("task_1 done");
}

async fn task_2 (m : &Mutex<i32>) {
    tokio::time::sleep(core::time::Duration::from_secs(2)).await;
    let _unused = m.lock().await;
    drop(_unused);
    println!("task_1 done");
}

@jobafr
Copy link

jobafr commented Apr 24, 2024

So, as I understand it now, that first sentence ("The feature that the async mutex offers over the blocking mutex is the ability to keep it locked across an .await point.") could be rephrased as "The feature that the async mutex offers over the blocking mutex is the ability to be locked over an await point without deadlocking the executor on concurrent attempts to lock the same mutex".

Now, since the docs also say "is ok and often preferred to use the ordinary Mutex from the standard library in asynchronous code", I wonder what the "correct" use case for std's mutex in async code is. Sharing something between sync code and async code?

tbodt added a commit to tbodt/tokio that referenced this issue Sep 4, 2024
@tbodt tbodt linked a pull request Sep 4, 2024 that will close this issue
@tbodt
Copy link

tbodt commented Sep 4, 2024

#6819

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-bug Category: This is a bug. M-sync Module: tokio/sync T-docs Topic: documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants