| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
| @@ -1,4 +1,5 @@ | |||
| import math | |||
| from threading import _PyRLock # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
why not from threading import RLock?
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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().
Sorry, something went wrong.
There was a problem hiding this comment.
nice! Sorry I missed that!
Sorry, something went wrong.
There was a problem hiding this comment.
No problem. I had to go re-read the threading module source to jog my memory 😄
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
Or maybe more easily just derive the class from RLock and implement locked() method using blocked=False as you suggest.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
That seems reasonable to me, it's only one place in tests so we don't need anything too fancy.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| @@ -1,4 +1,5 @@ | |||
| import math | |||
| from threading import _PyRLock # type: ignore[attr-defined] | |||
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
There was a problem hiding this comment.
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?
Sorry, something went wrong.
Signed-off-by: Ben Timby <btimby@gmail.com>
Signed-off-by: Ben Timby <btimby@gmail.com>
Signed-off-by: Ben Timby <btimby@gmail.com>
Signed-off-by: Ben Timby <btimby@gmail.com>
Signed-off-by: Ben Timby <btimby@gmail.com>
Signed-off-by: Ben Timby <btimby@gmail.com>
There was a problem hiding this comment.
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!
Sorry, something went wrong.
|
@csmarchbanks Thanks! I was not sure what to do and have not had a chance to research it. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.