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

rustc should suggest using async version of Mutex #71072

Open
jimblandy opened this issue Apr 12, 2020 · 12 comments
Open

rustc should suggest using async version of Mutex #71072

jimblandy opened this issue Apr 12, 2020 · 12 comments
Assignees
Labels
A-async-await A-diagnostics A-suggestion-diagnostics AsyncAwait-Triaged C-enhancement T-compiler

Comments

@jimblandy
Copy link
Contributor

@jimblandy jimblandy commented Apr 12, 2020

If one accidentally uses std::sync::Mutex in asynchronous code and holds a MutexGuard across an await, then the future is marked !Send, and you can't spawn it off to run on another thread - all correct. Rustc even gives you an excellent error message, pointing out the MutexGuard as the reason the future is not Send.

But people new to asynchronous programming are not going to immediately realize that there is such a thing as an 'async-friendly mutex'. You need to be aware that ordinary mutexes insist on being unlocked on the same thread that locked them; and that executors move tasks from one thread to another; and that the solution is not to make ordinary mutexes more complex but to create a new mutex type altogether. These make sense in hindsight, but I'll bet that they leap to mind only for a small group of elite users. (But probably a majority of the people who will ever read this bug. Ahem.)

So I think rustc should provide extra help when the value held across an await, and thus causing a future not to be Send, is a MutexGuard, pointing out that one must use an asynchronous version of Mutex if one needs to hold guards across an await. It's awkward to suggest tokio::sync::Mutex or async_std::sync::Mutex, but surely there's some diplomatic way to phrase it that is still explicit enough to be helpful.

Perhaps this could be generalized to other types. For example, if the offending value is an Rc, the help should suggest Arc.

Here's an illustration of what I mean:

use std::future::Future;
use std::sync::Mutex;

fn fake_spawn<F: Future + Send + 'static>(f: F) { }

async fn wrong_mutex() {
    let m = Mutex::new(1);
    let mut guard = m.lock().unwrap();
    (async { }).await;
    *guard += 1;
}

fn main() {
    fake_spawn(wrong_mutex());
    //~^ERROR: future cannot be sent between threads safely
}

The error message is great:

error: future cannot be sent between threads safely
  --> src/main.rs:14:5
   |
4  | fn fake_spawn<F: Future + Send + 'static>(f: F) { }
   |    ----------             ---- required by this bound in `fake_spawn`
...
14 |     fake_spawn(wrong_mutex());
   |     ^^^^^^^^^^ future returned by `wrong_mutex` is not `Send`
   |
   = help: within `impl std::future::Future`, the trait `std::marker::Send` is not implemented for `std::sync::MutexGuard<'_, i32>`
note: future is not `Send` as this value is used across an await
  --> src/main.rs:9:5
   |
8  |     let mut guard = m.lock().unwrap();
   |         --------- has type `std::sync::MutexGuard<'_, i32>`
9  |     (async { }).await;
   |     ^^^^^^^^^^^^^^^^^ await occurs here, with `mut guard` maybe used later
10 |     *guard += 1;
11 | }
   | - `mut guard` is later dropped here

I just wish it included:

help: If you need to hold a mutex guard while you're awaiting, you must use an async-aware version of the `Mutex` type.
help: Many asynchronous foundation crates provide such a `Mutex` type.

This issue has been assigned to @LucioFranco via this comment.

@jonas-schievink
Copy link
Member

@jonas-schievink jonas-schievink commented Apr 12, 2020

cc rust-lang/rust-clippy#4226

@steffahn
Copy link
Contributor

@steffahn steffahn commented Apr 12, 2020

Related open RFC: rust-lang/rfcs#2890

@Alexendoo Alexendoo added A-suggestion-diagnostics C-enhancement T-compiler A-diagnostics labels Apr 15, 2020
@Mark-Simulacrum Mark-Simulacrum added the A-async-await label Apr 29, 2020
@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Apr 29, 2020

"Must" is fairly strong here, it's not a strict requirement - you just might not get good performance and such if you do in fact hold this lock across the await point. There's discussion happening elsewhere (see the RFC thread, for example); since this is not a false-positive free lint, it may not belong in rustc proper. Given that a clippy lint has been implemented (rust-lang/rust-clippy#4226) I'm going to go ahead and close; if we want to move that lint from clippy to the compiler an RFC is probably the best way to do that.

@jimblandy
Copy link
Contributor Author

@jimblandy jimblandy commented Apr 29, 2020

@Mark-Simulacrum Actually, 'must' is correct. If one task locks a std::sync::Mutex and then awaits, and then another task tries to lock that same Mutex, if they happen to be on the same executor thread, then the mutex will either panic or deadlock. This is because std::sync::Mutex is not designed to be re-locked by the same thread that is holding it.

From the docs for Mutex::lock:

The exact behavior on locking a mutex in the thread which already holds the lock is left unspecified. However, this function will not return on the second call (it might panic or deadlock, for example).

I don't think this issue should be closed for this reason.

@Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Apr 29, 2020

Well, in that specific case, then yes, it's a bad idea. But you may be quite fine -- e.g. you only have a single task locking the mutex, so there's never a risk of it getting deadlocked or whatever.

At best we can try to say that if we see a mutexguard live across an await point (as in the error in your description), we can point out that an async mutex exists. I guess that could be helpful, so I'll reopen...

@tmandry tmandry added this to On deck in wg-async work via automation May 12, 2020
@tmandry tmandry added the AsyncAwait-Triaged label May 12, 2020
@tmandry
Copy link
Contributor

@tmandry tmandry commented May 12, 2020

Triage: This is an idea that has come up within wg-async-foundations. @LucioFranco last expressed interest in it, I think. Marking On Deck since I think it's something we definitely want.

@tmandry
Copy link
Contributor

@tmandry tmandry commented Jul 14, 2020

@rustbot assign @LucioFranco

@rustbot rustbot self-assigned this Jul 14, 2020
@tmandry tmandry moved this from On deck to In progress in wg-async work Jul 14, 2020
@LucioFranco
Copy link
Member

@LucioFranco LucioFranco commented Jul 14, 2020

We have an in-progress draft rfc here rust-lang/wg-async#16, would love feedback :)

@ryankurte
Copy link

@ryankurte ryankurte commented Mar 10, 2021

heya, just wondering whether there has been any discussion about a mutex-facade / custom-mutex approach similar to custom-allocators? this is perhaps not the right place for an extended discussion but, i believe it relates to the root cause of this issue.

one of the rather difficult to debug problems that seems to keep catching me is when a dependency uses the wrong type of mutex for the application, so you end up with a std::sync::Mutex putting your executor thread to sleep due to a call to an otherwise abstract library. a lint across everything compiled could help, but still leaves users needing to somehow fix the underlying issue of incorrect mutex use in direct or indirect dependencies. tangentially, on the embedded front we don't have std::sync::Mutex and have to support different mutex types which are currently addressed via a mutex trait we pass around, meaning libraries must be designed around this.

i'm sure it is more complex than one might expect, however, it would seem that a core::sync::Mutex trait (if we all could agree on an abstraction that works well for sync and async) coupled with a global mutex definition could mitigate both of these issues nicely, allowing the mutex type to be defined at project-level, and that there is a precedent for this approach with custom allocators?

@pcd1193182
Copy link

@pcd1193182 pcd1193182 commented Jul 14, 2021

I have a related question, and perhaps it should be a separate issue; is there some reason the compiler can't realize that if I call drop on a mutex before the await that it doesn't need to be sent? The ownership is gone (Drop takes the ownership, not a ref, and returns nothing), so I literally can't use it anymore, according to the borrow rules.

@dizda
Copy link

@dizda dizda commented Sep 20, 2021

I have the same problem as @pcd1193182

@tmandry
Copy link
Contributor

@tmandry tmandry commented Sep 21, 2021

That issue is tracked in #69663.

@dtolnay dtolnay assigned LucioFranco and unassigned rustbot Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await A-diagnostics A-suggestion-diagnostics AsyncAwait-Triaged C-enhancement T-compiler
Projects
None yet
Development

No branches or pull requests