Whether to automatically release the lock or not

fixed.
Same in Lock.php

same here

releases

fixed

@jderusse I think what you are tracking here is simply the ownership status of the current Lock object, which IMO should be renamed to LockHandler or something else less ambiguous with the lock which is the shared resource between many lock handlers.
So it would be great to rename "dirty" property to "owner" and add ownership concept to LockHandler API.

It will allows to:

• ensure that an owning lock handler can only release a lock (otherwise throws LogicException).
• ensure that a non-owning lock handler can only acquire a lock (otherwise throws LogicException).
• expose to the user an isOwner() method (!= isAcquired())

By the way this will also help to fix an issue I've found in CombinedStore implementation where you potentially release (as a cleaning step) acquired but not owned locks.

By design, the state of the lock is stored in the Key object. The Lock object is only a wraper and helper between the store and the key.
The following code works

$key = new Key(uniqid(__METHOD__, true));
$lock1 = new Lock($key, $store);
$lock2 = new Lock($key, $store);

$lock1->acquire();
$lock2->isAcquired(); // true

The purpose of this variable is not to track the ownership of the Lock, but to quickly know if the lock is acquired and avoid to call havy operation on the Lock destruction when the lock has not been acquired or has already been release (see comment #22132 (comment))

My point is just that ownership must be tracked to avoid ownership issues such as (for non reentrant lock, which is the case here):

• acquiring an already owned lock (i.e. already acquired by the current lock object).
• releasing a non owned lock (even and especially if owned by someone else).

This kind of issue can arise as long as program flaws exist, and when it happens throwing something like a LogicException is far more better than corrupt the system IMO.

And I think there is an ownership issue right here.

Unicity (or ownership) is tracked and stored in the Key by the store (A Key is not a resource)
You can't release a Lock with a different key.

The ownership is handled:

• by storing the filepointer in flock https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Store/FlockStore.php#L123
• by storing/and comparing a random value in Memcached and Redis https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Lock/Store/RedisStore.php#L105

Stores won't throw exception if you try to delete a key which is not the owner, but they neither don't deleted the lock.

$key1 = new Key('foo');
$key2 = new Key('foo');

$lock1 = new Lock($store, key1);
$lock2 = new Lock($store, key2);

$lock1->acquire(); // return true
$lock2->acquire(); // return false

$lock2->release(); // do nothing
$lock2->isAcquire(); // return false
$lock1->isAcquire(); // return true
$lock2->acquire(); // return false

Ok nice, but I'm just surprised to have ownership implemented on lock provider side (stores) since it could be simply and in a single place tracked in Lock class with a bool flag.

So the behavior here is indeed correct since non owned acquired locks will be silently discarded.
Correct but just still hard to understand IMO (I'd prefer explicitly checking isOwner() before calling release()).

And I still find very useful to have LogicException thrown when I make something illogic 😄

is the try catch actually required?

to not throw an exception when the auto-release try to release the lock...

• leave the try/cacth and you may miss exception when you count on the auto-release to release the lock

• remove the try/catch and you may have an unexpected exception when you manually release the lock and the auto-release retry to do it

isn't "release" idempotent?

indeed. Fixed

Whether to automatically release the lock or not when the lock instance is destroyed

when... (see previous comment)

one "if" is enough in the method:
if ($this->autoRelease && $this->dirty && $this->isAcquired()) { //...

unrelated, but just wondering: isn't this creating a race condition?

No, unless you share the key with an other process (i'm talking about the key, not the resource).

The method exists check both the "resource" and the "owner". You may have a race condition by using pcntl_fork (or serializeing then share the key) and both processes try to release and acquire the lock on the same time. But IMO, that's a weird case.

BTW, we don't provide methods to check if the lock is acquired by someone else.