in InterProcessLock, __init__ should be able to set default values arguments for acquire()

[MJuddBooth]

This would make its use as a context manager more flexible, eg with InterProcessLock(afile, delay=0.1, max_delay=1) as locked:

[psarka]

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.)

[MJuddBooth]

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.

[psarka]

Awesome, I will implement those 👍

[psarka]

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.

[MJuddBooth]

I see your point, and I think your proposed solution is reasonable - I certainly don't have a better idea. Thanks.

[pombredanne]

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

[psarka]

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

[pombredanne]

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!

[pombredanne]

And I have tested this at scale on ~ 20K runs and it looks fine so far! Thanks