Rust-clippy: Lint for using await while holding a MutexGuard

[tkaitchuck]

It would be useful to implement a lint against calling .await while a holding a MutexGuard.
This is almost certainly an error (and a non-obvious one) because the Reactor is going to end up invoking code not visible in the current scope while the lock is held.

[tmandry]

With a recent change to .await semantics (https://github.com/rust-lang/rust/pull/64292), the following pattern now holds a lock across await where it didn't before:

thing.lock().foo().await

This kind of lint may become more important for those used to the old behavior.

FYI, in Fuchsia, we had 8 instances of this in our codebase at the time of updating to a compiler with the new semantics. 6 of them were caught by compiler errors, but 2 weren't.

[nayato]

@tmandry I think the argument being made is that it is usually bad idea to take a lock and hold to across await. I'm making the same argument in #4893 in generic way.

[tmandry]

@tmandry I think the argument being made is that it is usually bad idea to take a lock and hold to across await. I'm making the same argument in #4893 in generic way.

Yes; I'm saying that this is easier to do accidentally now. I like your suggestion also.

[tmandry]

Implementation notes:

I think roughly what we want to do here is iterate over all async fns/async blocks in our crate (probably using intravisit), get their TypeckTables, and iterate over all types in generator_interior_types, checking for any types named MutexGuard.

Ideally this would support mutex implementations other than the standard library (such as parking_lot), which is I suggest checking the type name. We can also look for names like RwLockReadGuard and RwLockWriteGuard.

So far it's unclear to me whether parking_lot's reentrant locks should be included.

[flip1995]

Other lints about mutexes only lint on std types. I'd suggest to do the same here.

[tmandry]

@flip1995 that makes the lint less useful and catches fewer bugs. Anything named MutexGuard is bound to apply, so I think we should do it at least for the set of names std uses.

[rokob]

Hey I'm interested in taking this on. I will put together a WIP PR based on the guidance above and we can take it from there.

[tmandry]

@rustbot assign @rokob

[nayato]

@tmandry would it make sense to expand to Ref<T> and RefMut<T>? I think RefCell.borrow()/borrow_mut() being held across await point is as controversial and potentially more dangerous.
Of course it is limited to !Send futures only but still.

[tmandry]

@tmandry would it make sense to expand to Ref<T> and RefMut<T>? I think RefCell.borrow()/borrow_mut() being held across await point is as controversial and potentially more dangerous.
Of course it is limited to !Send futures only but still.

Yeah, probably! Though I'd be inclined to make it a separate lint. (I could be convinced otherwise)

[rokob]

So the PR is up and there was feedback to match against the full paths of the mutex types. I'm new to this codebase so I'm not sure how to arbitrate that, but I agree wit the above comment that doing so makes this lint much less useful. I made the change to match against full paths to see what it would look like, but I think switching back to just matching against anything named "MutexGuard" is more useful.

[goffrie]

Should this lint also be extended to RefCell borrows? It seems they are very similar in this regard.

[tmandry]

Should this lint also be extended to RefCell borrows? It seems they are very similar in this regard.

Yes, this was brought up earlier. I thought they should probably be separate lints, but don't have a strong opinion here. Please open an issue to discuss further.