Open jimblandy opened 4 years ago
Related open RFC: https://github.com/rust-lang/rfcs/pull/2890
"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 (https://github.com/rust-lang/rust-clippy/issues/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.
@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.
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...
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.
@rustbot assign @LucioFranco
We have an in-progress draft rfc here https://github.com/rust-lang/wg-async-foundations/pull/16, would love feedback :)
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?
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.
I have the same problem as @pcd1193182
That issue is tracked in #69663.
If one accidentally uses
std::sync::Mutex
in asynchronous code and holds aMutexGuard
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 theMutexGuard
as the reason the future is notSend
.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 beSend
, is aMutexGuard
, pointing out that one must use an asynchronous version ofMutex
if one needs to hold guards across an await. It's awkward to suggesttokio::sync::Mutex
orasync_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 suggestArc
.Here's an illustration of what I mean:
The error message is great:
I just wish it included:
This issue has been assigned to @LucioFranco via this comment.