Remove the force arg from release()

[ionelmc]

or something similar, eg:

• silent=True
• error=False
• regardless=True

[AndreiPashkin]

Two points:

1. Currently exception is not raised in case if unlock script didn't performed release because if id's mismatch.
2. 80% of functions can have parameter to suppress exceptions.

[ionelmc]

Maybe we should just remove this force argument ....

[AndreiPashkin]

Yes, that's what I'm proposing, also I propose to not track whether or not lock was held previously with self._held attribute, but track it by calling to Redis and comparing actual id with self._id, unlock script can do that and return exit codes.

[AndreiPashkin]

Another point is that extend() doesn't do an id check. So there is release() and reset(), but extend() and no hard_extend() or soft_extend(). What do you think?

[ionelmc]

I like this idea. I'd still like it to raise exception, just like threading.Lock does: https://docs.python.org/3/library/threading.html?highlight=raised#threading.Lock.release

[AndreiPashkin]

By exit codes I meant, that based on exit code from script method can raise exception or return successfully

[ionelmc]

Good point. extend() should raise error if lock ain't acquired, just like release().

[AndreiPashkin]

But should user be able to perform reset()-like extend(), without matching id?

[ionelmc]

I think reset is a special case. It's by design "unsafe" - it's throwing away all the state after all ... we just document that it's unsafe and should not be used in normal cases.

[ionelmc]

When reset was implemented initially it was for users that did a mess and forgot to properly release the locks. They needed a way to throw away all the locks (eg: after a machine crash, if they didn't use expires they need a way to have a "clean slate" on machine reboot)

[AndreiPashkin]

What if you need to extend a lock, that was held somewhere else?

[AndreiPashkin]

Another thing - what should extend() do if lock doesn't exist?

[ionelmc]

What if you need to extend a lock, that was held somewhere else?

The initial design was to have locks "id" as local state. IOW: "you lock it, you release it - it's your responsibility".

If, say, there are users who need to lock a resource during a course of multiple processes then they should manually carry over the state of the lock (the "id"). They can easily do that by creating Lock("name", id="12kjh34gjh312g4kj2h3g") in the right places and manually carry around the "id".

[ionelmc]

what should extend() do if lock doesn't exist

It's the same as if the lock wouldn't be acquired. You didn't lock it? You gotta get error cause there's something wrong in your code.

[AndreiPashkin]

In Python standard library there are Lock and RLock. The first is for cases, when you don't care, who created the lock, and the second is for cases, when you care. At least it tells, that cases, when people doesn't care about owner of lock, are exists.

[ionelmc]

As I understood it:

• This is fine:

lock = threading.Lock()
in thread1: lock.acquire()
in thread2: lock.release()

• This errors out on release():

lock = threading.RLock()
in thread1: lock.acquire()
in thread2: lock.release()

We could have an RLock object in redis-lock I suppose.

Now I realize the current implementation is like threading.RLock (barring the missing "counter"), and if you use .release(force=True) it works like threading.Lock (as long as you git it the right "id")

[ionelmc]

Ok I think we're derailing a bit, going a bit back:

• if lock ain't acquired by the current Lock instance then it must be an error.
• if lock was acquired elsewhere, but you created the lock with Lock(id="the correct value") it should not be an error.

Agree on those?

[AndreiPashkin]

I don't see clear solution, I need some time to think. Maybe it's also time to ask other contributors about what they are thinking.

[ionelmc]

Ok, so proposed new changes:

• changes to .release():

  • Check if lock was acquired with he same id. If not, raise error. Currently there's just a check if it was acquired with the same instance (self._held).

    This will allow locking a resource but sharing it between a group of processes that share the id. Note you can create locks with a given id like this: Lock("name", id=b'23a5b5a38e9f05587e944400148906f4').

  • Remove the force option. This isn't really necessary and it only encourages sloppy programming.

• changes to .extend():
  • Do the same checks as .release(). Raise NotAcquired in the same conditions as .release().

Anything against this @zoni @victor-torres @jweyrich ?

[victor-torres]

It's seems fine to me, @ionelmc.

• Sharing the same lock between threads looks good and;
• Users should be able to catch exceptions that were before silenced by force=True.

[AndreiPashkin]

What about adding force parameter to extend()?

[ionelmc]

In the last comment I proposed to remove the force arg completely. I have edited title now.

[AndreiPashkin]

Instead of removing force from release(), I propose to keep it, and also add force to extend().

[ionelmc]

What's wrong with having it raise exception instead?

[AndreiPashkin]

In my vision, it should work as you described, but also, when the methods invoked with force, they ignore id comparison.

[ionelmc]

There is .reset() exactly for that ...

What am I missing here? :)

[AndreiPashkin]

reset() is like release(force=True), but what about extend()?

[ionelmc]

I think it's totally OK to ask users to actually have the proper id when doing the extend.

[ionelmc]

So I've implemented the changes from #25 (comment) in #36

[ionelmc]

These changes will make couple of programming patterns be errors:

• release on expired lock is error
• acquire on lock with given id is error

I'm not 100% on the last one tho ...

[ionelmc]

The reasoning for "acquire on lock with given id is error" (eg: Lock(conn, "name", id="something")) around these two ideas:

• If you hardcode the id it's very likely that you are going to misuse the library. The point is that the id is supposed to be something unique. From the perspective of the user the value for the id should be an opaque value as the id represents something that is used internally for sanity checks.
• The target usecase for specifying the id is releasing a lock acquired else. Emphasis on "acquired". That means you don't need to acquire it ff you specify it.

Let me know if I missed something.

[victor-torres]

Nice.

[AndreiPashkin]

@ionelmc This makes impossible to use the same lock from different processes, like described in the #25 (comment) by @victor-torres.

[AndreiPashkin]

@ionelmc And here, you stated:

If you want to release a lock that you acquired in a different place you have two choices:
* Use Lock("name", id=id_from_other_place).release()

But how user can get id_from_other_place?

[ionelmc]

lock.id

On Friday, 19 February 2016, Andrew Pashkin notifications@github.com
wrote:

@ionelmc https://github.com/ionelmc And here
ecb405b#diff-6598a722fc587ba2a203c2037281e0e9R296,
you stated:

If you want to release a lock that you acquired in a different place you
have two choices:

• Use Lock("name", id=id_from_other_place).release()

But how user can get id_from_other_place?

—
Reply to this email directly or view it on GitHub
#25 (comment)
.

Thanks,
-- Ionel Cristian Mărieș, http://blog.ionelmc.ro

[AndreiPashkin]

@ionelmc Indeed, I thought there is no public interface for that.

[AndreiPashkin]

@ionelmc, what do you think about my first comment? I propose to remove that limitation. It will free user from requirement of having some sort of messaging for sharing actual lock id.

[ionelmc]

Not really sure what's being proposed here. Are you suggesting to remove the id completely? Then we don't have any way to protect from programming errors like: releasing what you didn't acquire, releasing expired lock, extending lock that isn't acquired by you and so on.

[AndreiPashkin]

No, I'm proposing to not treat lock as held if user just provided an id during initialization.

[ionelmc]

Hmmm. It could work. Could be check in __init__ if that id holds the lock?

[AndreiPashkin]

I think it's better to make a dedicated method for testing if lock is held. In my practice I had a case when I needed to make such tests and I just did .acquitre(blocking=False).

[ionelmc]

Ok but in this situation we'd have two kinds of "check if it's held":

1. Check if it's held with this id (and mark is as held on this Lock instance)
2. Check if it's held by other id

(1) would be new here

[AndreiPashkin]

I think now, that it would be good to reproduce how it's done in threading module - it has RLock, that can be released only by a process or thread, that have it acquired, and Lock, that can be released by any process/thread.

[AndreiPashkin]

Ok but in this situation we'd have two kinds of "check if it's held":

Check if it's held with this id (and mark is as held on this Lock instance)
Check if it's held by other id

(1) would be new here

I think, _held attribute should be removed at all, and each operation with a lock (acquire/release/etc) should make a call to Redis, to check if it's permitted to do the operation.

[AndreiPashkin]

_held makes no sense, because the actual state of lock is stored in Redis and it can change with time and caching it in a process memory is not what you want to do. You need Redis exactly because you don't want to store lock in local memory.

[ionelmc]

I think, _held attribute should be removed at all, and each operation with a lock (acquire/release/etc) should make a call to Redis, to check if it's permitted to do the operation.

I like this idea.

[AndreiPashkin]

@ionelmc, what do you think about RLock idea?

[ionelmc]

I still need to think about it. But on first look RLock on't make sense because we don't have any "counter". Not sure how this would be implemented, or if it's possible:

Once a process or thread has acquired a recursive lock, the same process or thread may acquire it again without blocking; that process or thread must release it once for each time it has been acquired.

[AndreiPashkin]

What RLock (and the cite) have to do with a counter?

[ionelmc]

As I understood it, you have to know how many times you acquired the lock. How do you interpret this?

Once a process or thread has acquired a recursive lock, the same process or thread may acquire it again without blocking; that process or thread must release it once for each time it has been acquired.

[AndreiPashkin]

Indeed, you're right, I never used recursive aspect of RLock in practice.

[AndreiPashkin]

My point is, that in different cases, different properties of synchronization mechanisms are needed, such as recursiveness or ability to be owned by a process. And for that, it is good to have different types of locks - recursive ownership-aware lock, recursive non-ownership-aware, etc.

[ionelmc]

I'm open to concrete proposals. BTW, shouldn't we have a different issue for sketching this out?

[AndreiPashkin]

I'm open to concrete proposals

Sounds good 👍