Add inspection for "with threading.Lock():"

[nathanielmanistaatgoogle]

Current problem

We've had a few instances of

with threading.Lock():
  <statements>

escape detection during review and find their way into our codebase; these are never correct or desired.

Desired solution

Would love for the code to be warned with with something like looks-like-mutual-exclusion-was-wanted-but-this-code-doesn't-achieve-mutual-exclusion or... a shorter, nicer warning name. 🙂

Additional context

For a file with contents

import threading

def not_a_helpful_function(parameter):
    with threading.Lock():
        print(parameter)

pylint-2.11.1 does not detect any logic problem in the code (it complains about the missing doc strings).

[Pierre-Sassoulas]

Maybe unsynchronizable-lock-declared-in-with or mutual-exclusion-need-shared-lock ? But do you really see this often ?

[nathanielmanistaatgoogle]

Maybe unsynchronizable-lock-declared-in-with or mutual-exclusion-need-shared-lock?

How about something really short: useless-lock?

But do you really see this often?

I asked myself that before opening this issue, and the answer to which I came was: no, not that often, but where we do see them, it's scary and severe, because of threading and concurrency. I suspect this is worth doing despite the low trigger rate because the value delivered is high enough and the cost of doing it is low enough, but I'm not certain.

[Pierre-Sassoulas]

I like useless-lock as a name, I'm also convinced by the argument value delivered is high enough and the cost of doing it is low enough. (This should be an easy implementation, maybe simply a visit_with + checking the name of the call).

Do you think it should be a default check @cdce8p ?

[cdce8p]

@nathanielmanistaatgoogle Could you explain why with won't work? I'm not an expert in threading but I think it should work. At least according to the documentation.

https://docs.python.org/3/library/threading.html#using-locks-conditions-and-semaphores-in-the-with-statement

Edit: Or do you mean that because it's create in place no other part can use it?

[DudeNr33]

Yes, the reason you gave in the Edit is exactly why this is dangerous.
The usage inside with is intended and even proposed by pylint through consider-using-with.
But a lock only does something useful if you use the same instance of the lock in all places which must not run concurrently.

I also think this check is useful, as programmers not used to threading might fall into this trap and conccurrency issues are always very time consuming to analyze, and might very well make their way into production until somebody notices.

[cdce8p]

Do you think it should be a default check @cdce8p ?

It would make sense to do so IMO. It's an error every time, even if rare, not something about coding style.
We could even consider creating a new checker class for it: ThreadingChecker (?) in case we want to extend it later with other checkers.