[Lock] Create a lock component #21093

jderusse · 2016-12-29T12:21:23Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	they will
Fixed tickets	#20382
License	MIT
Doc PR	symfony/symfony-docs#7364

This PR aim to add a new component Lock going further than the FileSystem\LockHandler by allowing remote backend (like Redis, memcache, etc)
Inspired by

Usage

The simplest way to use lock is to inject an instance of a Lock in your service

class MyService
{
    private $lock;

    public function __construct(LockInterface $lock)
    {
        $this->lock = $lock;
    }

    public function run()
    {
        $this->lock->acquire(true);

        // If I'm here, no exception had been raised. Lock is acquired
        try {
            // do my job
        } finally {
            $this->lock->release();
        }
    }
}

Configured with something like

services:
    app.my_service:
        class: AppBundle\MyService
        arguments:
            - app.lock.my_service
    app.lock.my_service:
        class: Symfony\Component\Lock\Lock
        factory: ['@locker', createLock]
        arguments: ['my_service']

If you need to lock serveral resource on runtime, wou'll nneed to inject the LockFactory.

class MyService
{
    private $lockFactory;

    public function __construct(LockFactoryInterface $lockFactory)
    {
        $this->lockFactory = $lockFactory;
    }

    public function run()
    {
        foreach ($this->items as $item) {
            $lock = $this->lockFactory->createLock((string) $item);

            try {
                $lock->acquire();
            } catch (LockConflictedException $e) {
                continue;
            }

            // When I'm here, no exception had been, raised. Lock is acquired
            try {
                // do my job
            } finally {
                $lock->release();
            }
        }
    }
}

Configured with something like

services:
    app.my_service:
        class: AppBundle\MyService
        arguments:
            - '@locker'

This component allow you to refresh an expirable lock.
This is usefull, if you run a long operation split in several small parts.
If you lock with a ttl for the overall operatoin time and your process crash, the lock will block everybody for the defined TTL.
But thank to the refresh method, you're able to lock for a small TTL, and refresh it between each parts.

class MyService
{
    private $lock;

    public function __construct(LockInterface $lock)
    {
        $this->lock = $lock;
    }

    public function run()
    {
        $this->lock->acquire(true);

        try {
            do {
                $finished = $this->performLongTask();

                // Increase the expire date by 300 more seconds
                $this->lock->refresh();
            } while (!$finished)
            // do my job
        } finally {
            $this->lock->release();
        }
    }
}

Naming anc implementation choise

$lock->acquire()
vs
$lock->lock()

Choose to use acquire, because this component is full of lock Symfony\Component\Lock\Lock::Lock` raised a E_TOO_MANY_LOCK in my head.

$lock->acquire(false);
$lock->acquire(true);
vs
$lock->aquire()
$lock->waitAndAquire()

Not a big fan of flag feature and 2. But I choose to use the blocking flag to offer a simple (and common usecase) implementation

$lock = $factory->createLock($key);
$lock->acquire();
vs
$lock->aquire($key)

I choose to a the pool of locks implementation. It allow the user to create 2 instances and use cross lock even in the same process.

interface LockInterface 
final class Lock implements LockInterface
vs
final class Lock

I choose to use a Interface even if there is only one implementaiton to offer an extension point here

TODO

In this PR

tests
add logs
offer several redis connectors
try other store implementation to validate the architecture/interface

In other PR

documentation
add configuration in framework bundle
add stop watch in the debug bar
improve the combined store (takes the drift into account and elapsed time between each store)
implement other stores (memcache, ...)
use this component in session manipulation (fixes [HttpFoundation] Session handlers should use a lock #4976)

lyrixx · 2016-12-29T12:30:07Z

When I worked on the Filesystem/LockHandler we (fab, niko, me) decided to not implement a "lock component" because it was always very specific to each use case: one vs multiple server. lock in redis with a ttl, Redlock, etc.

May be it's time to reconsider this and to introduce in symfony a new lock component.

Anyway, about the end user API:

I saw you arguments about aquire vs lock and for what is worth is prefer lock and release.

More over, I would prefer a system based on false/true than on exception.

andersonamuller · 2016-12-29T12:56:37Z

Since you said the component is already full of Lock names, what about call the component Mutex?

linaori

Some minor points, architectural I think it looks fine. I'm not too experienced with locking itself, but this component is similar an in-house Mutex component my company wrote, thus I do like it a lot.

linaori · 2016-12-29T13:18:50Z

src/Symfony/Component/Lock/Lock.php

+            throw $e;
+        } catch (\Exception $e) {
+            throw new LockAcquiringException('', 0, $e);
+        }


Shouldn't you be catching \Throwable as well?

I wonder if it make sens to wrap every exception with "LockException" here?
Or I should just let the original exception been thrown.

If yes, you're right, I should catch everything.
If no, I can remove this statement.

WDYT?

linaori · 2016-12-29T13:19:02Z

src/Symfony/Component/Lock/Lock.php

+            throw $e;
+        } catch (\Exception $e) {
+            throw new LockAcquiringException('', 0, $e);
+        }


Shouldn't you be catching \Throwable as well?

on Error, it should let the error throw imho. throwing a LockAcquiringException would make no sense. Error is a dev mistake, not a runtime one.

Fair enough, I wanted to make sure this was considered

linaori · 2016-12-29T13:19:38Z

src/Symfony/Component/Lock/Lock.php

+            } else {
+                throw new InvalidArgumentException(
+                    sprintf('Unable to acquire a blocking lock on a instance of "%s".', get_class($this->store))
+                );


I think this should go on 1 line

linaori · 2016-12-29T13:21:20Z

src/Symfony/Component/Lock/Store/CombinedStore.php

+ *
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ */
+class CombinedStore implements StoreInterface, ExpirableStoreInterface


Should probably be made final

linaori · 2016-12-29T13:21:28Z

src/Symfony/Component/Lock/Quorum/UnanimousQuorum.php

+ *
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ */
+class UnanimousQuorum implements QuorumInterface


Should probably be made final

linaori · 2016-12-29T13:33:46Z

src/Symfony/Component/Lock/Lock.php

+use Symfony\Component\Lock\Store\ExpirableStoreInterface;
+
+/**
+ * Lock is the default implementation to the LockInterface.


implementation to of the LockInterface

linaori · 2016-12-29T13:35:31Z

src/Symfony/Component/Lock/LockInterface.php

+{
+    /**
+     * Acquire the lock. If the lock is acquired by someone else, the parameter `blocking` determine whether or not
+     * the the call should block until the release of the lock.


the parameter blocking determines whether

linaori · 2016-12-29T13:35:52Z

src/Symfony/Component/Lock/LockInterface.php

+     * Acquire the lock. If the lock is acquired by someone else, the parameter `blocking` determine whether or not
+     * the the call should block until the release of the lock.
+     *
+     * @param bool  $blocking whether or not the Lock should wait the release of someone else


should wait for the release of

linaori · 2016-12-29T13:39:10Z

src/Symfony/Component/Lock/QuorumInterface.php

+     * Returns Whether or not the quorum *could* be met.
+     * This method does not means the quorum *would* be met for sur. But can be useful to early stop a process when you
+     * known there isn't any chance to met the quorum.
+     *


I think there should be a blank line between the main and sub description

This method does not means the quorum *would* be met for ~~sur. But~~ sure, but can be useful to ~~early~~ stop a process early when you known there ~~isn't any~~ is no chance to meet the quorum.

linaori · 2016-12-29T13:39:49Z

src/Symfony/Component/Lock/Store/BlockingStoreInterface.php

+interface BlockingStoreInterface
+{
+    /**
+     * Wait a key became free then stores the resource.


Wait a key ~~became~~ becomes free**,** then stores the resource.

ro0NL

Agree on the finals. Im not into locking myself as well.. at first it looked slightly over-engineered.. then again i could understand we're super flexible here in terms of supporting various complex strategies.

I think i'd prefer true/false API as well.

And not sure.. but cant we do closures somehow to streamline the process?

ro0NL · 2016-12-29T13:47:16Z

src/Symfony/Component/Lock/Store/RetryTillSaveStore.php

+    public function __construct(StoreInterface $decorated, $retrySleep = 50, $retryCount = PHP_INT_MAX)
+    {
+        $this->decorated = $decorated;
+        $this->retryCount = $retryCount;


Perhaps use $retryCount ?: PHP_INT_MAX and go 0 in the construct. To keep up with the promise like @iltar mentioned 👍

javiereguiluz · 2016-12-29T14:39:57Z

Regarding the naming of the methods, I've been checking the practices of other languages:

Python:

lock = Lock()
lock.acquire()       # waits if lock is already held
lock.acquire(False)  # doesn't wait if lock is already held
lock.release()
if lock.locked()

Ruby:

lock = Mutex.new
lock.lock           # waits if lock is already held
lock.try_lock       # doesn't wait if lock is already held
lock.unlock
if lock.locked?

PHP pthreads extension

$lock = Mutex::create();
Mutex::lock($lock);     // waits if lock is already held
Mutex::trylock($lock);  // doesn't wait if lock is already held
Mutex::unlock($lock);
// no method to check if the lock is locked

Java:

Lock lock = ...;
lock.lock();        // waits if lock is already held
lock.tryLock();     // doesn't wait if lock is already held
lock.unlock();
// no method to check if the lock is locked

It looks like the most common choice is: lock, tryLock and unlock

nicolas-grekas · 2016-12-29T15:00:00Z

src/Symfony/Component/Lock/TokenGenerator/RandomTokenGenerator.php

+     */
+    public function generateToken()
+    {
+        return base64_encode(random_bytes($this->entropy / 8));


I suggest inlining this and remove RandomTokenGenerator and TokenGeneratorInterface: they are just internal details.

nicolas-grekas · 2016-12-29T15:03:25Z

src/Symfony/Component/Lock/LockFactory.php

+     */
+    public function createLock($resource)
+    {
+        return new Lock(new Key($resource), $this->store);


what about removing the factory and move createLock to the store interface?

nicolas-grekas · 2016-12-29T15:08:57Z

src/Symfony/Component/Lock/Store/FlockStore.php

+        );
+
+        // Silence error reporting
+        set_error_handler(function () {


single line

nicolas-grekas · 2016-12-29T15:09:02Z

src/Symfony/Component/Lock/Store/FlockStore.php

+            return;
+        }
+
+        $fileName = sprintf(


single line

Is there a limit to the line length in CS?

jderusse · 2016-12-29T20:43:19Z

Moved the createLock factory in the StoreInterface
Create an AbstractClass to avoid dupplication of code (Could be a trait too, WDYT)
Remove the TokenGenerator

API choices:

IMO lock and unlock fit well when the subject is the resource. ie. $resource->lock() or $handler->lock($resource);
In this PR the subject is not the resource, but the Lock itself, in this case, I think the python naming is meaningful $lock->acquire/release.

I like the split in lock/tryLock, I think I'll apply this change unless someone prefer the $blocking flag?

About the return true/false vs \Exception. Personally, I see it like "lock me the resource right now" instead of "can you please lock the resource". If the lock is not acquired, it means sometinhg goes wrong, that's why I excpect an exception.
But that's just my taste/opinion

fabpot · 2016-12-30T05:58:32Z

src/Symfony/Component/Lock/Key.php

+        $this->resource = (string) $resource;
+    }
+
+    public function getResource()


__toString()?

fabpot · 2016-12-30T05:58:55Z

src/Symfony/Component/Lock/Key.php

+    }
+
+    /**
+     * @return mixed


Can be removed as it does not say anything useful

fabpot · 2016-12-30T06:00:43Z

src/Symfony/Component/Lock/Lock.php

+            $this->store->expire($this->key, $ttl);
+            $this->logger->info('Expiration defined for lock "{resource}" for "{ttl}" seconds', array('resource' => $this->key->getResource(), 'ttl' => $ttl));
+        } catch (LockConflictedException $e) {
+            $this->logger->info('Fail to define a expiration for the lock "{resource}". Someone else acquired the lock', array('resource' => $this->key->getResource()));


an expiration, same below.

I would use a single sentence here instead of 2.

Sorry, I don't see how to merge this two sentences without removing a part (which are both important IMO) :(

Failed to define an expiration for the "{resource}" lock because someone else acquired it?

fabpot · 2016-12-30T06:01:29Z

src/Symfony/Component/Lock/Lock.php

+            $this->logger->info('Fail to define a expiration for the lock "{resource}". Someone else acquired the lock', array('resource' => $this->key->getResource()));
+            throw $e;
+        } catch (\Exception $e) {
+            $this->logger->info('Fail to define a expiration for the lock "{resource}"', array('resource' => $this->key->getResource(), 'exception' => $e));


the {resource} lock.

fabpot · 2016-12-30T06:01:55Z

src/Symfony/Component/Lock/LockInterface.php

+interface LockInterface
+{
+    /**
+     * Acquire the lock. If the lock is acquired by someone else, the parameter `blocking` determines whether or not


Acquires, same everywhere else.

fabpot · 2016-12-30T06:02:43Z

src/Symfony/Component/Lock/QuorumInterface.php

+interface QuorumInterface
+{
+    /**
+     * Returns Whether or not the quorum is met.


extra cap (same below)

fabpot · 2016-12-30T06:04:19Z

src/Symfony/Component/Lock/Store/FlockStore.php

+        );
+
+        // Silence error reporting
+        set_error_handler(function () {


fabpot · 2016-12-30T06:05:45Z

src/Symfony/Component/Lock/Store/RetryTillSaveStore.php

+
+/**
+ * RetryTillSaveStore is a StoreInterface implementation which decorate a non blocking StoreInterface to provide a
+ * kind of blocking storage.


"a kind of" should probably be removed.

fabpot · 2016-12-30T06:06:21Z

src/Symfony/Component/Lock/StoreInterface.php

+    public function exists(Key $key);
+
+    /**
+     * Retrieves a lock for the given resource.


It retrieves or it creates?

fabpot · 2016-12-30T06:12:36Z

src/Symfony/Component/Lock/Lock.php

+    /**
+     * {@inheritdoc}
+     */
+    public function acquire($blocking = false, $ttl = 300.0)


What about moving the default ttl value to the constructor instead?

The purpose of this parameter was to let the user choose a specific duration for one call.

Given the easiest (and probably the most used) workflow would be to use the Store to create the Lock, it does not make sens to move this parameter in the __constructor: If we want a fixed ttl value, we can just remove it and let the store use what he want.

I'll replace the default value 300.0 by a null. It let the user to override the default store configuration.

Finally move the parameter in the constructor (as suggested) and remove it from the refresh method too (because this method serves the same purpose).

fabpot · 2016-12-30T06:12:56Z

src/Symfony/Component/Lock/Lock.php

+    /**
+     * {@inheritdoc}
+     */
+    public function refresh($ttl = 300.0)


I would make the ttl argument required, without a default value.

jderusse · 2016-12-30T18:51:00Z

src/Symfony/Component/Lock/Store/RetryTillSaveStore.php

+
+                return;
+            } catch (LockConflictedException $e) {
+                usleep($this->retrySleep * 1000);


Should add randomness to avoid eternal collision

thewilkybarkid · 2017-01-01T10:19:46Z

src/Symfony/Component/Lock/Exception/InvalidArgumentException.php

+namespace Symfony\Component\Lock\Exception;
+
+/**
+ * InvalidArgumentException is thrown when a service had been badly initialized.


If it has a specific usage, the name should reflect it.

jderusse · 2017-01-01T19:14:34Z

Pushed a new commit to remove the 2 interfaces BlockingStoreInterface and ExpirableStoreInterface and merge the methods into StoreInterface.

At first it was a god thing to split it 3 interfaces to separate responsibilities, but I realize that it was a pain to decorate the stores because we both need to implement every interfaces and are not sure that the decorated store will implement every interfaces. In this case, the decorator don't know what to do.

fabpot · 2017-01-10T22:51:28Z

@jderusse you need to also add the new component in the replace key of the root composer.json file.

fabpot · 2017-01-10T22:52:10Z

src/Symfony/Component/Lock/LICENSE

@@ -0,0 +1,19 @@
+Copyright (c) 2016 Fabien Potencier


should be updated to 2016-2017 I suppose

pierredup · 2017-01-11T12:22:36Z

src/Symfony/Component/Lock/Lock.php

+ *
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ */
+final class Lock implements LockInterface


Should this not implement LoggerAwareInterface as well?

stof · 2017-01-11T12:30:19Z

src/Symfony/Component/Lock/Store/AbstractStore.php

+     */
+    public function createLock($resource, $ttl = 300.0)
+    {
+        return new Lock(new Key($resource), $this, $ttl);


making stores create the lock by passing $this to the Lock instance makes it very hard to use composition because the Lock instance must always be created by the outside store in this case instead of delegating the call to createLock (otherwise, the lock has the wrong store, as it ends up calling directly the inner store).

IMO, the Store should not be the lock factory at the same time. Mixing these responsibilities is what makes things hard.

We should either have a separate factory (in which the store is injected, and so decoration works fine as the factory would get the outside store) or let the instantiation be done in userland when using the library.

That was the first implementation (I like the separation of responsibility too).
But, as suggested by @nicolas-grekas in this comment I move the factory into the StoreInterface and that's why I create the AbstractStore to avoid code duplication (which I don't really like too).

The issue with the factory is was an empty class like without real value:

class StoreFactory { private $store; public function __construct(StoreInterface $store) { $this->store = $store; } public function createLock($resource, $ttl = 300.0) { return new Lock(new Key($resource), $this->lock, $ttl); } }

And IMO, the outer store may create the lock instead of delegating the creation to the inner store. The abstractStore do the job (maybe a trait could be a better solution than the abstraction)

What do you think ?

(just to be sure, you are talking about decoration instead of composition ?)

stof · 2017-01-11T12:30:54Z

src/Symfony/Component/Lock/Store/CombinedStore.php

+            try {
+                $store->delete($key);
+            } catch (\Exception $e) {
+            } catch (\Throwable $e) {


catching all exceptions silently would be a debugging nightmare

Fixed, but with a notice level given the deletion failure may not be an issue

stof · 2017-01-11T12:31:28Z

src/Symfony/Component/Lock/Store/CombinedStore.php

+                $store->putOffExpiration($key, $ttl);
+                ++$successCount;
+            } catch (\Exception $e) {
+                ++$failureCount;


we are loosing all exceptions messages here, making debugging really hard

stof · 2017-01-11T12:36:50Z

src/Symfony/Component/Lock/Tests/Store/BlockingStoreTestTrait.php

+
+    /**
+     * Tests blocking locks thanks to pcntl.
+     *


missing @requires extension pcntl

I wasn't aware of this feature. Thanks. I also annotate \Redis tests

stof · 2017-01-11T12:37:07Z

src/Symfony/Component/Lock/Tests/Store/BlockingStoreTestTrait.php

+        $drift = 20000;
+
+        if (PHP_VERSION_ID < 50600 || defined('HHVM_VERSION_ID')) {
+            $this->markTestSkipped('Php 5.5 does not keep resource in child forks');


message is wrong in the case of HHVM

stof · 2017-01-11T12:40:45Z

src/Symfony/Component/Lock/Tests/Store/RetryTillSaveStoreTest.php

+
+    public function getStore()
+    {
+        $redis = new \Predis\Client('tcp://'.getenv('REDIS_HOST').':6379');


what if there is no such env variable ?

This part of the test is mostly a copy/paste of the RedisAdapterTest from the Cache component.
The variable is define is the phpunit.xml file and should exists.

I don't think it worst it to check whether or not someone destroy the config file and expect a successful result.

jjsaunier · 2017-01-11T12:49:12Z

I think a little explanation about clock drift (which is introduced) could be a plus, a link in code comment, or in the docs to automatically answer about the "why" when people look at the code could be useful:)

jderusse · 2017-01-11T20:37:52Z

@Prophet777 You're absolutely right, but this PR don't yet introduce a clock drift imlplementation.

I'll do it in a next PR or in a other PR

Wirone

My 2 (literally) cents.

Wirone · 2017-03-22T21:26:39Z

src/Symfony/Component/Lock/Store/SemaphoreStore.php

+     */
+    public function putOffExpiration(Key $key, $ttl)
+    {
+        // do nothing, the flock locks forever.


Invalid comment, probably copy-pasted.

Wirone · 2017-03-22T21:32:05Z

src/Symfony/Component/Lock/Store/RedisStore.php

+        }
+
+        // Have to be a \Predis\Client
+        return call_user_func_array(array($this->redis, 'eval'), array_merge(array($script, 1, $resource), $args));


I know that constructor allows only some classes and most of them are used above, but wouldn't be more readable to check instance of \Predis\Client too? After all if statements exception could be thrown that script could not be evaluated.

Fixed in #22117
Thank you @Wirone

goetas · 2017-03-24T07:38:21Z

Probably I'm late, but in the past I've worked with an almost identical lock model, and it almost never worked.

If the process responsible for releasing the lock dies, most of the time the lock needs to be released manually (deleting the lock file or the redis record). The timeout mitigates the problem but does not solve it.

goetas · 2017-03-24T07:31:59Z

src/Symfony/Component/Lock/Lock.php

+    {
+        $this->store->delete($this->key);
+
+        if ($this->store->exists($this->key)) {


In a concurrent environment, another process might have created the same lock here. exists will return true even if the previous delete was successful.

If the lock had been acquired by an other instance of key, the store will treat them like 2 different keys and will return false.

ah, i got it. there is a "token" in between, that is always random, for the same key you have different "tokens"

goetas · 2017-03-24T07:33:46Z

src/Symfony/Component/Lock/LockInterface.php

+     * @throws LockConflictedException If the lock is acquired by someone else in blocking mode
+     * @throws LockAcquiringException  If the lock can not be acquired
+     */
+    public function acquire($blocking = false);


To avoid concurrency issues this should return a unique reference for the lock, so the release should work by using this reference

This unique reference is in the key. By passing the key to the methods save/delete/exists the concurrency is already handled

jocel1 · 2017-03-24T08:38:47Z

What about adding a MysqlStore using GET_LOCK() / RELEASE_LOCK() / IS_FREE_LOCK() functions ?
https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_get-lock

It's robust and available in all MySQL versions.

jderusse · 2017-03-24T09:50:53Z

@goetas Then I've got a good news to you, the actual implementation does not suffer for such issue. Thanks to flock (or equivalent mechanism in other stores) the system kernel will automatically free the lock even if you forget to release it, or die, or segfault, ...

@jocel1 I didn't dig into MySQL store, but it's on my todo list. Do you want to create that PR?

goetas · 2017-03-24T09:54:25Z

@jocel1 In mysql there is a fundamental concept that PHP has not, the connection.

As the documentation says:

A lock obtained with GET_LOCK() is released explicitly by executing RELEASE_LOCK() or implicitly when your session terminates (either normally or abnormally)

At the end of the connection, the lock will be automatically released.

jocel1 · 2017-03-24T09:57:16Z

@goetas yup that's why it also great to easily handle the dying script case: there's nothing specific to do :)

goetas · 2017-03-24T09:59:25Z

src/Symfony/Component/Lock/Store/MemcachedStore.php

+     */
+    public function __construct(\Memcached $memcached, $initialTtl = 300)
+    {
+        if (!static::isSupported()) {


is it possible to have an object instance of \Memcached and not having the memcached extension loaded?

@jderusse ?

It's technically possible that someone implement a \Memcached class.
If the class cover the original extension behaviors and is compatible, everything'll be fine.
If not, that's a weird case and should be addressed IMO.

goetas · 2017-03-24T10:03:09Z

@jderusse Can you point me what guarantees that delete gets called for the memcache store as example? (https://github.com/jderusse/symfony/blob/48c998a097e8c71d90c5e8b508d21c8b866f2db1/src/Symfony/Component/Lock/Store/MemcachedStore.php#L113)

goetas · 2017-03-24T10:57:58Z

I have tested the Flock and the Redis store.

With Flock, if the scripts terminates anomaly, the lock gets released by the kernel, but for redis this does not happen. I guess that memcache suffers from the same problem.

$predis = new \Predis\Client('tcp://localhost:6379');
$store = new \Symfony\Component\Lock\Store\RedisStore($predis);
$factory = new \Symfony\Component\Lock\Factory($store);

$lock = $factory->createLock('foo');
if ($lock->acquire()) {
    if (!rand(0, 1)) exit; // simulating failure
    $lock->release();
}

After the failure occurs, the only way to unlock the lock, is to remove manually the entry from redis.

jderusse · 2017-03-24T10:58:38Z

@goetas None. But I can guarantee that Memcache will delete the key bat it own, when the TTL will expire.

@jocel1 it could be an issue to me. What if the connection closes while you think you are in lock phase (network issue, or mysql read timeout, ...)
Second issue. how do you know if you are the locker?

goetas · 2017-03-24T11:01:17Z

@jderusse

None. But I can guarantee that Memcache will delete the key bat it own, when the TTL will expire

i knew this, the standard TTL that many key-value store have

jderusse · 2017-03-24T11:03:33Z

I suggest to read the (not yet merged) documentation about this https://github.com/symfony/symfony-docs/pull/7364/files

goetas · 2017-03-24T11:09:51Z

@jderusse thanks for the explanation.

The documentation is clear and highlights the differences between different stores (advantages and disadvantages). At the beginning I was thinking that the all the stores implementations have to fulfill all the requirements (event when hardy possible)

goetas · 2017-03-24T11:11:05Z

Mysql and postgres locks can be interesting to implement since they offer both advantages of local and remote locks :)

stof · 2017-03-24T11:18:29Z

src/Symfony/Component/Lock/Lock.php

+            return false;
+        } catch (\Exception $e) {
+            $this->logger->warning('Failed to lock the "{resource}".', array('resource' => $this->key, 'exception' => $e));
+            throw new LockAcquiringException('', 0, $e);


@jderusse I suggest using a non-empty exception message for better DX

jocel1 · 2017-03-24T11:40:34Z

@jderusse

it could be an issue to me. What if the connection closes while you think you are in lock phase (network issue, or mysql read timeout, ...)
Second issue. how do you know if you are the locker?

If the connection closes on the locker side, it could indeed be problematic if you're not making subsequent requests on the db (in this case, the subsequent query would fail as well). But I would expect this to occur far less often than a TTL timeout or even an eviction prior to key expiry on memcached.
For the second issue, if you're the locker, you can call another GET_LOCK, the call will not be blocking. It's only blocking if you're calling the GET_LOCK in another session.

patrick-mcdougle · 2017-04-10T01:30:03Z

Can we eliminate the memcached store? Memcached is notorious for evicting keys that aren't even to their exptime because they are LRU than something else in the same slab class. By having it, developers will trust it more than they should. It's great as a cache, but really shouldn't be used as a persistent concurrent locking mechanism. Thoughts?

jderusse · 2017-04-10T04:42:13Z

Hi @patrick-mcdougle, can you please open an issue in order to follow this point?

This PR was merged into the master branch. Discussion ---------- Add docs for the Lock component See symfony/symfony#21093 Commits ------- 3035fe9 Rename quorum into strategy d4326c0 Reduce usage of "you" d6a218c Rename quorum strategy 17248e1 Remove codeblock 6b6d865 Final review and some minor rewords/simplifications 2498ccd Fix typo d1d3b71 Fixed minor typos and syntax issues 60bfe03 Fix typo 6469016 Add the Lock Component

…ry (corphi) This PR was merged into the 3.4 branch. Discussion ---------- [Lock][PhpDoc] There is no attempt to create the directory | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - When the lock component [was](#21093) [created](#22597), the `FlockStore` implementation was copied from the filesystem component. But the dependency to its origin was dropped, and as part of that, the attempt to create the parent directory for the locks. The PhpDoc wasn’t updated to reflect this change. Commits ------- 93a9bd3 PhpDoc: There is no attempt to create the directory

carsonbot added Status: Needs Review Feature Deprecation labels Dec 29, 2016

linaori suggested changes Dec 29, 2016

View reviewed changes

ro0NL reviewed Dec 29, 2016

View reviewed changes

nicolas-grekas reviewed Dec 29, 2016

View reviewed changes

fabpot reviewed Dec 30, 2016

View reviewed changes

jderusse force-pushed the lock branch from 2596340 to 3251ed5 Compare December 30, 2016 17:27

jderusse commented Dec 30, 2016

View reviewed changes

thewilkybarkid reviewed Jan 1, 2017

View reviewed changes

jderusse force-pushed the lock branch from 04c3f34 to af96dd7 Compare January 1, 2017 19:14

nicolas-grekas added this to the 3.x milestone Jan 2, 2017

fabpot reviewed Jan 10, 2017

View reviewed changes

jderusse force-pushed the lock branch 2 times, most recently from 816523b to f5b20bd Compare January 10, 2017 23:17

pierredup reviewed Jan 11, 2017

View reviewed changes

stof reviewed Jan 11, 2017

View reviewed changes

jderusse force-pushed the lock branch from 4e23948 to c48619b Compare January 12, 2017 22:35

fabpot closed this Mar 22, 2017

Tobion mentioned this pull request Mar 22, 2017

[HttpFoundation] Session handlers should use a lock #4976

Open

Wirone reviewed Mar 22, 2017

View reviewed changes

goetas reviewed Mar 24, 2017

View reviewed changes

stof reviewed Mar 24, 2017

View reviewed changes

nicolas-grekas modified the milestone: 3.x Mar 24, 2017

patrick-mcdougle mentioned this pull request Apr 16, 2017

[Lock] [RFC] Deprecate/Remove Memcached Store #22452

Closed

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

corphi mentioned this pull request Apr 29, 2018

[Lock][PhpDoc] There is no attempt to create the directory #27089

Merged

jderusse deleted the lock branch August 2, 2019 12:16

jderusse mentioned this pull request Dec 27, 2020

[Lock] Add LockFactoryInterface #39634

Closed

[Lock] Create a lock component #21093

[Lock] Create a lock component #21093

Conversation

jderusse commented Dec 29, 2016 • edited Loading

Usage

Naming anc implementation choise

TODO

In this PR

In other PR

lyrixx commented Dec 29, 2016 • edited Loading

andersonamuller commented Dec 29, 2016

linaori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL left a comment • edited Loading

Choose a reason for hiding this comment

ro0NL Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

javiereguiluz commented Dec 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jderusse commented Dec 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jderusse commented Jan 1, 2017

fabpot commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjsaunier commented Jan 11, 2017 • edited Loading

jderusse commented Jan 11, 2017

Wirone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jderusse commented Dec 29, 2016 •

edited

Loading

lyrixx commented Dec 29, 2016 •

edited

Loading

nicolas-grekas Dec 29, 2016 •

edited

Loading

ro0NL left a comment •

edited

Loading

ro0NL Dec 29, 2016 •

edited

Loading

jderusse commented Dec 29, 2016 •

edited

Loading

jjsaunier commented Jan 11, 2017 •

edited

Loading

goetas Mar 24, 2017 •

edited

Loading