8000 [Lock] Automaticaly release lock when user forget it by jderusse · Pull Request #22132 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Lock] Automaticaly release lock when user forget it #22132

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

[Lock] Automaticaly release lock when user forget it #22132

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61f66d6
Automaticaly release lock on destruction
jderusse Mar 23, 2017
14ebf6a
Add a private variable to store lock state
jderusse Mar 31, 2017
70a01ac
Configurable autorelease
jderusse Jul 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Configurable autorelease
  • Loading branch information
@jderusse
jderusse committed Sep 28, 2017
commit 70a01ac2aa3ec21f3a1f6ee9b83756596c946f09
9 changes: 5 additions & 4 deletions src/Symfony/Component/Lock/Factory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,15 @@ public function __construct(StoreInterface $store)
/**
* Creates a lock for the given resource.
*
* @param string $resource The resource to lock
* @param float $ttl Maximum expected lock duration in seconds
* @param string $resource The resource to lock
* @param float $ttl Maximum expected lock duration in seconds
* @param bool $autoRelease Whether to automatically release the lock or not when the lock instance is destroyed
*
* @return Lock
*/
public function createLock($resource, $ttl = 300.0)
public function createLock($resource, $ttl = 300.0, $autoRelease = true)
{
$lock = new Lock(new Key($resource), $this->store, $ttl);
$lock = new Lock(new Key($resource), $this->store, $ttl, $autoRelease);
$lock->setLogger($this->logger);

return $lock;
Expand Down
23 changes: 12 additions & 11 deletions src/Symfony/Component/Lock/Lock.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,34 +32,35 @@ final class Lock implements LockInterface, LoggerAwareInterface
private $store;
private $key;
private $ttl;
private $autoRelease;
private $dirty = false;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😄


/**
* @param Key $key Resource to lock
* @param StoreInterface $store Store used to handle lock persistence
* @param float|null $ttl Maximum expected lock duration in seconds
* @param Key $key Resource to lock
* @param StoreInterface $store Store used to handle lock persistence
* @param float|null $ttl Maximum expected lock duration in seconds
* @param bool $autoRelease Whether to automatically release the lock or not when the lock instance is destroyed
*/
public function __construct(Key $key, StoreInterface $store, $ttl = null)
public function __construct(Key $key, StoreInterface $store, $ttl = null, $autoRelease = true)
{
$this->store = $store;
$this->key = $key;
$this->ttl = $ttl;
$this->autoRelease = (bool) $autoRelease;

$this->logger = new NullLogger();
}

/**
* Automatically release the underlying lock when the object is destructed.
* Automatically releases the underlying lock when the object is destructed.
*/
public function __destruct()
{
try {
if ($this->dirty && $this->isAcquired()) {
$this->release();
}
} catch (\Throwable $e) {
} catch (\Exception $e) {
if (!$this->autoRelease || !$this->dirty || !$this->isAcquired()) {
return;
}

$this->release();
}

/**
Expand Down
21 changes: 20 additions & 1 deletion src/Symfony/Component/Lock/Tests/LockTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ public function testIsAquired()
->expects($this->any())
->method('exists')
->with($key)
->willReturn(true);
->will($this->onConsecutiveCalls(true, false));

$this->assertTrue($lock->isAcquired());
}
Expand Down Expand Up @@ -150,6 +150,25 @@ public function testReleaseOnDestruction()
unset($lock);
}

public function testNoAutoReleaseWhenNotConfigured()
{
$key = new Key(uniqid(__METHOD__, true));
$store = $this->getMockBuilder(StoreInterface::class)->getMock();
$lock = new Lock($key, $store, 10, false);

$store
->method('exists')
->willReturnOnConsecutiveCalls(array(true, false))
;
$store
->expects($this->never())
->method('delete')
;

$lock->acquire(false);
unset($lock);
}

/**
* @expectedException \Symfony\Component\Lock\Exception\LockReleasingException
*/
Expand Down
0