Open MJuddBooth opened 3 years ago
Interesting! Do you have a usecase, where you need different values of delay and max_delay? I thought the defaults should work almost always, so if anything, I would have suggested to make these two parameters even less exposed.
(On the other hand, timeout parameter should be part of the context manager API.)
Sorry, I could have been clearer and chosen better examples. What I really had in mind was cases like with InterProcessLock(afile, blocking=False)
or InterProcessLock(afile, timeout=2)
as you suggest. I don't really see a need to fiddle with delay or max_delay.
Awesome, I will implement those :+1:
Actually, I now realized that this situation is quite ugly. The thing is, blocking
and timeout
are not features of the lock, but rather parameters of an attempt to get the lock. Some functions may want to try to acquire the same lock with timeout, some without.
For that reason, the blocking
and timeout
parameters rightly belong to the .acquire
method, as it is implemented.
At the same time, not having them in the context manager API is very annoying, so we will have to find a way to solve it. I checked the threading.Lock
in the standard library to see how they do this, and they don't. They do not offer timeout
and blocking
in context manager setting :(
And it is natural that they don't, as their approach of having the lock object itself (rather than a method) to serve as entry point does not allow for it. So my solution would be to leave the current sweet (and unhealthy) sugar
lock = Lock()
with lock:
...
as it is, and add a more flexible (and healthier) carrot for those that prefer it:
lock = Lock()
with lock.locked(timeout, blocking):
...
This would mirror the ReaderWriter lock, that has entry points as follows:
rw_lock = ReaderWriterLock()
with rw_lock.read_locked():
...
with rw_lock.write_locked():
...
I'll leave this here to gather some feedback for a week or so.
I see your point, and I think your proposed solution is reasonable - I certainly don't have a better idea. Thanks.
FWIW, I am considering switching from https://github.com/yougov/yg.lockfile which offers the timeout
feature in a context manager and that would be a blocker for me not to have this.
I use it this way
https://github.com/nexB/scancode-toolkit/blob/b14ca7b2af3ee09486d9cf704752118db5d75064/src/licensedcode/cache.py#L222
This feature is definitely on the list, but I currently I don't like the API options for it :(
I'll note that while I'm stuck, the exception raising one is very easy to make yourselves on top of lock by just adding the method. Like this:
from contextlib import contextmanager
from fasteners import InterProcessLock
class FailedToAcquireException(Exception):
pass
class WithTimeout(InterProcessLock):
@contextmanager
def locked(self, timeout):
ok = self.acquire(timeout=timeout)
if not ok:
raise FailedToAcquireException()
try:
yield
finally:
self.release()
try:
with WithTimeout('zzz').locked(100):
... # exclusive access
except FailedToAcquireException:
... # manage failure
This feature is definitely on the list, but I currently I don't like the API options for it :(
I'll note that while I'm stuck, the exception raising one is very easy to make yourselves on top of lock by just adding the method. Like this:
from contextlib import contextmanager from fasteners import InterProcessLock class FailedToAcquireException(Exception): pass class WithTimeout(InterProcessLock): @contextmanager def locked(self, timeout): ok = self.acquire(timeout=timeout) if not ok: raise FailedToAcquireException() try: yield finally: self.release() try: with WithTimeout('zzz').locked(100): ... # exclusive access except FailedToAcquireException: ... # manage failure
@psarka Thanks! that's simple enough. I also appreciate that fasteners has no external deps!
And I have tested this at scale on ~ 20K runs and it looks fine so far! Thanks
This would make its use as a context manager more flexible, eg
with InterProcessLock(afile, delay=0.1, max_delay=1) as locked: