Use re-entrant lock.

[btimby]

While using django-prometheus I found a situation where adding a metric to the registry caused a read from cache. I had cache metrics enabled, which attempted to add cache metrics to the registry. This resulting in a deadlock as the second registry call attempted to obtain the lock already held lower on the call stack.

I changed usage to an RLock, which allows the lock to be obtained in a nested fashion within a call stack. This still provides thread safety.

[MRigal]

why not from threading import RLock?

[btimby]

RLock does not have a locked() method.

https://docs.python.org/3/library/threading.html#rlock-objects

However, _PyRLock provides _count which I am using to provide that method to satisfy the rest of the code's usage of the lock.

[btimby]

Look closely at threading, RLock is a factory function that either returns an instance of _CRLock or _PyRLock. The former does not have a _count attribute though the latter does. I am choosing _PyRLock explicitly in order to implement locked().

[MRigal]

nice! Sorry I missed that!

[btimby]

No problem. I had to go re-read the threading module source to jog my memory 😄

[csmarchbanks]

Unless I am missing something only tests depend on locked(). Could we instead use RLock in the code, and change the test to check the lock in a different manner? E.g. trying to acquire the lock with blocked=false and testing if we acquired the lock or not.

[btimby]

Or maybe more easily just derive the class from RLock and implement locked() method using blocked=False as you suggest.

[btimby]

Well, nevermind, as I mentioned above, RLock is a factory function (can't derive). Could instead have a locked(lock) -> bool function used in tests.

[csmarchbanks]

That seems reasonable to me, it's only one place in tests so we don't need anything too fancy.

[csmarchbanks]

I have concerns with using _PyRLock as it appears that both behavior may change between cpython and python, and at least some people appear to want to deprecate or remove the function in Python 3.13: https://discuss.python.org/t/should-we-deprecate-threading-pyrlock-in-3-13/27297.

[csmarchbanks]

Unless I am missing something only tests depend on locked(). Could we instead use RLock in the code, and change the test to check the lock in a different manner? E.g. trying to acquire the lock with blocked=false and testing if we acquired the lock or not.

[btimby]

@csmarchbanks Only one test needed to check the status of the lock. Therefore, I simply switched all locks to RLock and added a utility function (in the test file) for that test.

[csmarchbanks]

The new implementation looks good to me! However the diffs show changes from other recently merged PRs. Any chance you could clean those up with a rebase?

[csmarchbanks]

I force pushed your branch to remove the extraneous commits/diffs so this is in a mergeable state now. No other changes were made.

Thanks!

[btimby]

@csmarchbanks Thanks! I was not sure what to do and have not had a chance to research it.