[Lock] Include lock component in framework bundle #22113

jderusse · 2017-03-22T21:29:43Z

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

Usage

use the best available "out of box" store (semaphore if available, filesyste otherwise)

framework:
    lock: ~
    # lock: true

$this->get('lock')->acquire();
$this->get('lock.factory')->createLock('my resource')->acquire();

use a specific store

framework:
    lock: flock
    # lock: semaphore
    # lock: redis://localhost
    # lock: "%env(MEMCACHED_DSN)%"
    # lock: ["%env(REDIS_DSN_1)%", "%env(REDIS_DSN_2)%"]

$this->get('lock')->acquire();
$this->get('lock.factory')->createLock('my resource')->acquire();

use a named lock

framework:
    lock: 
        foo: flock
        bar: redis://localhost

$this->get('lock.foo')->acquire();
$this->get('lock.bar.factory')->createLock('my resource')->acquire();

factory usage

        <service id="acme" class="AppBundle\Acme">
            <argument type="service">
                <service class="Symfony\Component\Lock\Lock">
                    <factory service="lock.foo.factory" method="createLock" />
                    <argument>my resource</argument>
                    <argument>30</argument> <!-- optional TTL -->
                </service>
            </argument>
        </service>

Tests

jderusse · 2017-03-22T21:33:21Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/lock.xml

+
+        <service id="lock.store.combined.abstract" class="Symfony\Component\Lock\Store\CombinedStore" abstract="true" public="false">
+            <argument /> <!-- List of stores -->
+            <argument type="service" id="lock.quorum.majority" /> <!-- Quorum -->


Could be rename into strategy

fbourigault · 2017-03-23T10:13:54Z

What about adding a framework.lock.default config to override the lock.factory priority order? So the user can choose the implementation he wants as default.

javiereguiluz · 2017-03-23T10:46:51Z

Maybe I'm late, but I'd like to propose a simpler way to use and configure this component inside the Symfony Framework. The idea, as usual, is to optimize/simplify the common case and don't complicate much the complex case.

Common case

Application defines/needs just 1 lock type.

Configuration

This is the default config, where Symfony picks the best lock type for you: semaphore if the extension is installed; flock otherwise:

framework:
    lock: ~

This is the explicit config where you set the lock type (string or array and some smart conversion by the Config component, as we do everywhere else):

framework:
    lock: 'flock'
    lock: 'semaphore'
    lock: 'memcached://m1.docker'
    lock: ['memcached://m1.docker', 'memcached://m2.docker']
    lock: 'redis://r1.docker'
    lock: ['redis://r1.docker', 'redis://r2.docker']

Usage

// non-blocking, non-expiring
$lock = $this->get('lock')->acquire();
// $lock = $container->get('lock')->acquire();

// blocking, non-expiring
$lock = $this->get('lock')->acquire(true);

// non-blocking, expiring in 60 seconds
$lock = $this->get('lock')->acquire(false, 60);

// normal operations on the lock
$lock->isAcquired();
$lock->refresh();
$lock->release();

Complex case

Application defines and uses several lock types.

Configuration

framework:
    lock:
        users: ['memcached://m1.docker', 'memcached://m2.docker']
        admin: 'flock'
        partners: 'redis://r1.docker'
        invoices: 'semaphore'

Usage

$adminLock = $this->get('lock.admin')->acquire();
$invoicesLock = $this->get('lock.invoices')->acquire(true);
$partnersLock = $this->get('lock.partners')->acquire(false, 30);
// ...

chalasr · 2017-03-23T13:07:29Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+
+                if (count($hostsDefinitions) > 1) {
+                    $definition = new ChildDefinition('lock.store.combined.abstract');
+                    $definition->replaceArgument(0, $hostsDefinitions);


Would it make sense to lazy load stores using an iterator? It seems the combined store can break in the middle of the store chain. Btw, why not ChainStore instead of CombinedStore for consistency?

ro0NL · 2017-03-23T19:46:49Z

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

+
+            if (null !== $username || null !== $password) {
+                if (!method_exists($client, 'setSaslAuthData')) {
+                    trigger_error('Missing SASL support: the memcached extension must be compiled with --enable-memcached-sasl.');


What about a E_USER_WARNING? As we're clearly going to call an undefined method.

Edit: i see it's a copy paste of the cache component.. understandable. Probably not worth it then. Im not sure this is desirable maintenance-wise though (vs. soft-depending on symfony/cache).

I choose to use the same error/format than src/Symfony/Component/Cache/Traits/MemcachedTrait.php for more consistency

jderusse · 2017-04-01T20:05:08Z

@javiereguiluz Today I start to implement your suggestion (much simpler, I like it).
But then, I realize, it'll not be compatible with dynamic resources.
I suggest a small change in your proposal by adding factories too.

$this->get('lock_factory.invoices')->createLock('invoices.'.$invoice->getId());

the service lock.invoices would be an alias of $this->get('lock_factory.invoices')->createLock('invoices').
WDYT?

I've got some other question:

When you says $lock = $this->get('lock')->acquire(); you means $lock = $this->get('lock') + $isLocked = $this->get('lock')->acquire(); right?

Last point you are moving the ttl from the lock constructor to the acquire method. Is it made on purpose? Shouldn't it be a parameter defined in the config file like:

framework:
    lock:
        users: ['memcached://m1.docker', 'memcached://m2.docker']
        invoices:
            store: ['memcached://m1.docker', 'memcached://m2.docker']
            ttl: 300

nicolas-grekas · 2017-04-28T17:46:35Z

Moving to milestone 3.4 as this is not a blocker for 3.3. If you don't agree, please tell :)

jderusse · 2017-07-31T06:27:38Z

PR rebased

chalasr · 2017-07-31T11:57:09Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/lock.xml

+    xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+    <services>
+


<defaults public="false" />

chalasr · 2017-07-31T11:57:57Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            if ('default' === $resourceName) {
+                $container->setAlias('lock.store', new Alias('lock.'.$resourceName.'.store'));
+                $container->setAlias('lock.factory', new Alias('lock.'.$resourceName.'.factory'));
+                $container->setAlias('lock', new Alias('lock.'.$resourceName));


should be private unless strongly needed

I moved .store services to private. But keep lock and lock.factory public. To let user retrieve the lock from a ContainerAware object (controller, command, ...)

$this->get('lock.foo')->acquire(); $this->get('lock.bar.factory')->createLock('my resource')->acquire(); $this->get('lock')->acquire(); $this->get('lock.factory')->createLock('my resource')->acquire();

id I missed something? Or am I using it wrongly?

ContainerAware objects are still supported, but I think we all agree that they should not be promoted (for end applications esp.) and features should not be designed with that in mind.
Better inject @lock.factory (controllers/commands/everything as services, should be autoinjected quite easily also) rather than locate it using a DIC (here is the bad habit: a DIC is not meant to be used as a service locator but to wire services, see #21623 for reference).

If no internal need for get() this service, we should keep it private (regarding BC we can make it public afterwards if legit use case, not the inverse).

chalasr · 2017-07-31T12:00:06Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            // Generate factories for each resource
+            $factoryDefinition = new ChildDefinition('lock.factory.abstract');
+            $factoryDefinition->replaceArgument(0, new Reference('lock.'.$resourceName.'.store'));
+            $factoryDefinition->setPublic(true);


unless strongly needed it should remain private

chalasr · 2017-07-31T12:09:59Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                $combinedDefinition->replaceArgument(0, $storeDefinitions);
+                $container->setDefinition('lock.'.$resourceName.'.store', $combinedDefinition);
+            } else {
+                $container->setAlias('lock.'.$resourceName.'.store', new Alias((string) $storeDefinitions[0]));


chalasr · 2017-07-31T12:10:11Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                            $container->setDefinition($connectionDefinitionId, $connectionDefinition);
+                        }
+
+                        $storeDefinition = new Definition(\stdClass::class);


StoreInterface::class?

jderusse · 2017-08-10T07:35:22Z

@chalasr tests should be fixed by #23847

chalasr · 2017-08-10T07:36:15Z

Cool! thanks

chalasr · 2017-08-18T09:31:36Z

ping @symfony/deciders

nicolas-grekas · 2017-09-05T07:50:54Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+    {
+        $loader->load('lock.xml');
+
+        $container->getDefinition('lock.store.flock')->replaceArgument(0, sys_get_temp_dir());


This inlines the tempdir used at compile time for reuse at build time, making the dumped container not-independent from the build env.

nicolas-grekas · 2017-09-05T07:52:51Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                        $storeDefinition = new Reference('lock.store.semaphore');
+                        break;
+                    case $usedEnvs || preg_match('#^[a-z]++://#', $storeDsn):
+                        if (!$container->hasDefinition($connectionDefinitionId = md5($storeDsn))) {


$container->hash() instead of md5

nicolas-grekas · 2017-09-05T07:54:10Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                            $container->setDefinition($connectionDefinitionId, $connectionDefinition);
+                        }
+
+                        $storeDefinition = new Definition(StoreInterface::class);


the definition should me made private

nicolas-grekas · 2017-09-05T07:54:26Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+                        $storeDefinition->setFactory(array(StoreFactory::class, 'createStore'));
+                        $storeDefinition->setArguments(array(new Reference($connectionDefinitionId)));
+
+                        $container->setDefinition($storeDefinitionId = 'lock.'.$resourceName.'.store.'.md5($storeDsn), $storeDefinition);


$container->hash also here

nicolas-grekas · 2017-09-05T07:54:33Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+
+                        $storeDefinition = new Reference($storeDefinitionId);
+                        break;
+                    case $usedEnvs:


dead code because of the previous case

nicolas-grekas · 2017-09-05T07:55:47Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            $container->setDefinition('lock.'.$resourceName.'.factory', $factoryDefinition);
+
+            // Generate services for lock instances
+            $lockDefinition = new Definition(Lock::class);


should be private

nicolas-grekas · 2017-09-05T07:57:19Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

+            if ('default' === $resourceName) {
+                $container->setAlias('lock.store', new Alias('lock.'.$resourceName.'.store', false));
+                $container->setAlias('lock.factory', new Alias('lock.'.$resourceName.'.factory', false));
+                $container->setAlias('lock', new Alias('lock.'.$resourceName, false));


missing FQCN aliases that could be use by autowiring

nicolas-grekas · 2017-09-05T07:59:22Z

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

+     *
+     * @throws \ErrorEception When invalid options or dsn are provided
+     */
+    public static function createConnection($dsn, array $options = array())


should be updated to match #23978

jderusse · 2017-09-05T22:39:02Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/lock.xml

+
+        <service id="lock.store.flock" class="Symfony\Component\Lock\Store\FlockStore">
+            <argument type="service">
+                <service class="string">


I've some doublt about the value of class here

nicolas-grekas · 2017-09-18T15:32:41Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/lock.xml

+        <service id="lock.store.flock" class="Symfony\Component\Lock\Store\FlockStore">
+            <argument type="service">
+                <service class="string">
+                    <factory function="sys_get_temp_dir"/>


instead of this, what about making the $lockPath arg of FlockStore optional, and make it use sys_get_temp_dir then?

nicolas-grekas · 2017-09-18T15:33:34Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/lock.xml

+            </argument>
+        </service>
+
+        <service id="lock.store.semaphore" class="Symfony\Component\Lock\Store\SemaphoreStore"/>


bad consistency about space before /> in the file

nicolas-grekas · 2017-09-18T15:37:22Z

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

+     *
+     * @return \Redis|\Predis\Client According to the "class" option
+     */
+    public static function createConnection($dsn, array $options = array())


We could maybe consider a new component, named Dsn, with factories for any kind of DSN we want to support?
not sure we can make it for 3.4, but what do others think about the idea?

nicolas-grekas

👍

nicolas-grekas · 2017-09-19T07:28:26Z

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

@@ -32,12 +32,15 @@ class FlockStore implements StoreInterface
    private $lockPath;

    /**
-     * @param string $lockPath the directory to store the lock
+     * @param string|null $lockPath the directory to store the lock. Default values will use temporary directory


the directory to store the lock, defaults to the system's temporary directory

chalasr

fine tuning

chalasr · 2017-09-26T21:57:35Z

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

+    public static function createConnection($dsn, array $options = array())
+    {
+        if (!is_string($dsn)) {
+            throw new InvalidArgumentException(sprintf('The %s() method expect argument #1 to be string, %s given.', __METHOD__, gettype($dsn)));


chalasr · 2017-09-26T22:04:46Z

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

+ */
+class StoreFactory
+{
+    public static function createConnection($dsn, array $options = array())


I think we should add a docblock listing supported dsn

will be removed replaced by the new Dns component

chalasr · 2017-09-26T22:08:50Z

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

+        throw new InvalidArgumentException(sprintf('Unsupported DSN: %s.', $dsn));
+    }
+
+    public static function createStore($connection)


fabpot · 2017-09-27T15:12:45Z

Thank you @jderusse.

…russe) This PR was squashed before being merged into the 3.4 branch (closes #22113). Discussion ---------- [Lock] Include lock component in framework bundle | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | symfony/symfony-docs#8066 ## Usage use the best available "out of box" store (semaphore if available, filesyste otherwise) ```yml framework: lock: ~ # lock: true ``` ```php $this->get('lock')->acquire(); $this->get('lock.factory')->createLock('my resource')->acquire(); ``` use a specific store ```yml framework: lock: flock # lock: semaphore # lock: redis://localhost # lock: "%env(MEMCACHED_DSN)%" # lock: ["%env(REDIS_DSN_1)%", "%env(REDIS_DSN_2)%"] ``` ```php $this->get('lock')->acquire(); $this->get('lock.factory')->createLock('my resource')->acquire(); ``` use a named lock ```yml framework: lock: foo: flock bar: redis://localhost ``` ```php $this->get('lock.foo')->acquire(); $this->get('lock.bar.factory')->createLock('my resource')->acquire(); ``` factory usage ```xml <service id="acme" class="AppBundle\Acme"> <argument type="service"> <service class="Symfony\Component\Lock\Lock"> <factory service="lock.foo.factory" method="createLock" /> <argument>my resource</argument> <argument>30</argument>  </service> </argument> </service> ``` * [x] Tests Commits ------- b4b00c9 [Lock] Include lock component in framework bundle

This PR was merged into the 3.4 branch. Discussion ---------- [Lock] Integrate framework Should probably be merged after #7866 Documentation for symfony/symfony#22113 Commits ------- 142e703 Fixed minor issues 7f1e087 Add lock configuration in framework bundle

jderusse commented Mar 22, 2017

View reviewed changes

jderusse force-pushed the lock-framework branch from 8c0a97b to c696f0a Compare March 22, 2017 22:58

jderusse changed the title ~~[Lock] Include lock component in framework bundle~~ [WIP] [Lock] Include lock component in framework bundle Mar 22, 2017

chalasr reviewed Mar 23, 2017

View reviewed changes

ro0NL reviewed Mar 23, 2017

View reviewed changes

jderusse force-pushed the lock-framework branch from c696f0a to 8417f2d Compare March 23, 2017 19:49

nicolas-grekas added this to the 3.3 milestone Mar 24, 2017

jderusse force-pushed the lock-framework branch from 8417f2d to 9d24eb1 Compare April 1, 2017 10:02

jderusse force-pushed the lock-framework branch from 9d24eb1 to b513ba6 Compare April 28, 2017 05:29

nicolas-grekas modified the milestones: 3.3, 3.4 Apr 28, 2017

jderusse force-pushed the lock-framework branch from b513ba6 to 279342f Compare May 1, 2017 20:01

jderusse force-pushed the lock-framework branch from 279342f to 2c0db9f Compare June 19, 2017 19:55

jderusse changed the base branch from master to 3.4 June 19, 2017 19:56

jderusse force-pushed the lock-framework branch 7 times, most recently from ef4c5de to 68954c3 Compare June 19, 2017 21:11

jderusse changed the title ~~[WIP] [Lock] Include lock component in framework bundle~~ [Lock] Include lock component in framework bundle Jun 19, 2017

jderusse mentioned this pull request Jun 19, 2017

[Lock] Integrate framework symfony/symfony-docs#8066

Merged

jderusse force-pushed the lock-framework branch from 68954c3 to c341129 Compare July 31, 2017 06:12

chalasr reviewed Jul 31, 2017

View reviewed changes

fabpot approved these changes Aug 23, 2017

View reviewed changes

nicolas-grekas reviewed Sep 5, 2017

View reviewed changes

jderusse force-pushed the lock-framework branch 2 times, most recently from d65375a to 34a22f9 Compare September 5, 2017 22:34

jderusse commented Sep 5, 2017

View reviewed changes

nicolas-grekas reviewed Sep 18, 2017

View reviewed changes

jderusse force-pushed the lock-framework branch 3 times, most recently from 52420e9 to 06342f4 Compare September 19, 2017 06:28

nicolas-grekas approved these changes Sep 19, 2017

View reviewed changes

jderusse force-pushed the lock-framework branch from 06342f4 to 518e44b Compare September 19, 2017 07:31

chalasr approved these changes Sep 26, 2017

View reviewed changes

jderusse added 3 commits September 27, 2017 07:32

Include lock component in framework bundle

f5d91f5

New config format

776200f

Reduce lock service visibility

0b9c2e6

jderusse force-pushed the lock-framework branch from 518e44b to 0b9c2e6 Compare September 27, 2017 05:36

fabpot approved these changes Sep 27, 2017

View reviewed changes

fabpot closed this Sep 27, 2017

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

danrot mentioned this pull request Jul 25, 2019

Avoid parallel generation of images sulu/sulu#4617

Closed

2 tasks

jderusse deleted the lock-framework branch August 2, 2019 12:17

		xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">

		<services>

[Lock] Include lock component in framework bundle #22113

[Lock] Include lock component in framework bundle #22113

Conversation

jderusse commented Mar 22, 2017 • edited Loading

Usage

Choose a reason for hiding this comment

fbourigault commented Mar 23, 2017 • edited Loading

javiereguiluz commented Mar 23, 2017 • edited Loading

Common case

Configuration

Usage

Complex case

Configuration

Usage

Choose a reason for hiding this comment

ro0NL Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jderusse commented Apr 1, 2017

nicolas-grekas commented Apr 28, 2017

jderusse commented Jul 31, 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

chalasr Jul 31, 2017 • 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

jderusse commented Aug 10, 2017

chalasr commented Aug 10, 2017

chalasr commented Aug 18, 2017

nicolas-grekas Sep 5, 2017 • 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

nicolas-grekas Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr 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

Choose a reason for hiding this comment

fabpot commented Sep 27, 2017

jderusse commented Mar 22, 2017 •

edited

Loading

fbourigault commented Mar 23, 2017 •

edited

Loading

javiereguiluz commented Mar 23, 2017 •

edited

Loading

ro0NL Mar 23, 2017 •

edited

Loading

chalasr Jul 31, 2017 •

edited

Loading

nicolas-grekas Sep 5, 2017 •

edited

Loading

nicolas-grekas Sep 18, 2017 •

edited

Loading