[Console] Add Lockable trait #18471

geoffrey-brier · 2016-04-07T10:57:29Z

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	none for the moment :)

Hi there,

Since the 2.6 the LockHandler class was added to ease concurrency problems. There was a nice post about using it in your commands.

From my humble experience, I find it a bit unpleasant/time consuming to always copy/paste the same code. So here is my proposal:

Before:

class UpdateContentsCommand extends Command
{
    protected function configure()
    {
        // ...
    }

    protected function execute(InputInterface $input, OutputInterface $output)
    {
        // create the lock
        $lock = new LockHandler('update:contents');
        if (!$lock->lock()) {
            $output->writeln('The command is already running in another process.');

            return 0;
        }

      // Your code

        $lock->release();
    }
}

After:

class MyCommand extends Command
{
    use LockableTrait;

    protected function execute(InputInterface $input, OutputInterface $output)
    {
         if (!$this->lock()) {
             // Here you can handle locking errors
         }
        // Your code
        // The lock release is still optionnal
         $this->release();
    }
}

In addition, you can optionally pass two arguments:

a string argument to change the lock name
a boolean argument to indicate if you want to wait until the requested lock is released

linaori · 2016-04-07T12:39:20Z

src/Symfony/Component/Console/Command/Command.php

+     */
+    public function setAutoLock($autoLock)
+    {
+        $this->autoLock = (boolean) $autoLock;


I think the standard is to use (bool), though usually the values are not cast like that afaik

javiereguiluz · 2016-04-07T13:18:17Z

About the proposed method name: ->setAutoLock(true); I don't think it's bad, but maybe we can improve it.

My concern: the "auto-lock" feature is an "implementation detail". What you really want is to prevent concurrent command executions. You don't care about the lock at all.

If you agree, the hard part will be to find a good name for this.

geoffrey-brier · 2016-04-07T13:54:59Z

@javiereguiluz I do agree with you, I wasn't really satisfied with the naming ... what about "safe lock", "thread safe" ... ? Or setParallelizable ?

linaori · 2016-04-07T14:13:05Z

The only problem with this locking strategy is that it only works if your command is limited to 1 run per application instance. If I would deploy 2 applications which both run the command, it could cause issues.

geoffrey-brier · 2016-04-07T14:27:52Z

@iltar That's true (though the problem also occurs before this PR). What would you suggest ? Introduce some kind of interface with various strategies ? If that's the case that may be a good idea to create a new "locking" component.

javiereguiluz · 2016-04-07T14:30:20Z

@iltar @geoffrey-brier I think that'd a reasonable limitation of this feature.

linaori · 2016-04-07T14:31:33Z

If it's extendable, it's really nice (I think I've suggested this in the original feature but not sure).

It's already a nice to have feature as is though.

geoffrey-brier · 2016-04-07T14:35:52Z

@javiereguiluz I think it would be reasonable too (may be be nice to enhance it later though).

fabpot · 2016-04-28T10:52:59Z

ping @lyrixx

lyrixx · 2016-04-28T11:50:08Z

I like the idea
I agree with @javiereguiluz comments
But I don't really like the implementation. I think we should stick to what is described in the blog post. The diff will be smaller, and the code easier to understand
Should we add a note about the limitation (1 server) ?

fabpot · 2016-06-14T06:23:03Z

@geoffrey-brier Do you have some time to finish this PR by taking comments into account? Thanks.

geoffrey-brier · 2016-06-14T07:33:23Z

@fabpot Sure no problem but I'm not sure to understand correctly.

Should I add another method ? What about the naming ?

geoffrey-brier · 2016-06-14T10:05:35Z

@fabpot @javiereguiluz @iltar I've completely reworked the PR.

Now you would do something like this:

class MyCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output)
    {
         // By default it would throw a RuntimeException if the command is locked
         $this->lock();
        // Your code
        // The lock release is still optionnal
         $this->release();
    }
}

You can also pass a boolean to the lock method so that it doesn't throw any exception.

class MyCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output)
    {
         // Silently return
         if ($this->lock(true)) {
             return;
         }
    }
}

fabpot · 2016-06-23T05:35:53Z

src/Symfony/Component/Console/Command/LockableTrait.php

+     *
+     * @throws RuntimeException
+     */
+    protected function lock($name, $blocking = false)


Do we really need the name here? As the trait is only able to deal with only one lock anyway, I'd suggest to remove the name and generate one automatically.

You could have a name depending on some arguments. But a default value (the command name) is indeed better.

Fait enough, let's add a default value then.

What about making the name able to accept full paths as well? We usually put our locks into %projectdir%/var/run so doing something like

$lockPath = null; $lockName = $name ?: $this->getName(); if ($name && $fs->isAbsolutePath($name)) { $lockPath = dirname($name); $lockName = basename($name); }

would be great. Or is that look too much?

geoffrey-brier · 2016-06-23T09:32:56Z

Fixed everything

fabpot · 2016-06-23T09:35:22Z

👍

1ed · 2016-06-23T09:40:28Z

src/Symfony/Component/Console/composer.json

@@ -21,6 +21,7 @@
    },
    "require-dev": {
        "symfony/event-dispatcher": "~2.8|~3.0",
+        "symfony/filesystem": "",


Missing version.

lyrixx · 2016-06-23T09:45:22Z

@geoffrey-brier Could you update the PR description to have something that match the new implementation? Thanks.

nicolas-grekas · 2016-06-23T09:49:37Z

src/Symfony/Component/Console/Command/LockableTrait.php

+     */
+    protected function lock($name = null, $blocking = false)
+    {
+        if (!class_exists('Symfony\Component\Filesystem\LockHandler')) {


LockHandler::class

geoffrey-brier · 2016-06-23T11:36:21Z

@lyrixx Done & tests are green :)

fabpot · 2016-06-23T11:41:47Z

@geoffrey-brier Looks good. Can you add a new item in the component CHANGELOG to mention this new feature for 3.2?

geoffrey-brier · 2016-06-23T11:43:17Z

@fabpot Done

fabpot · 2016-06-23T11:47:33Z

Thank you @geoffrey-brier.

This PR was merged into the 3.2-dev branch. Discussion ---------- [Console] Add Lockable trait | 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 | none for the moment :) Hi there, Since the 2.6 the `LockHandler` class was added to ease concurrency problems. There was a nice post about [using it in your commands](http://symfony.com/blog/new-in-symfony-2-6-lockhandler). From my humble experience, I find it a bit unpleasant/time consuming to always copy/paste the same code. So here is my proposal: Before: ```php class UpdateContentsCommand extends Command { protected function configure() { // ... } protected function execute(InputInterface $input, OutputInterface $output) { // create the lock $lock = new LockHandler('update:contents'); if (!$lock->lock()) { $output->writeln('The command is already running in another process.'); return 0; } // Your code $lock->release(); } } ``` After: ```php class MyCommand extends Command { use LockableTrait; protected function execute(InputInterface $input, OutputInterface $output) { if (!$this->lock()) { // Here you can handle locking errors } // Your code // The lock release is still optionnal $this->release(); } } ``` In addition, you can optionally pass two arguments: - a string argument to change the lock name - a boolean argument to indicate if you want to wait until the requested lock is released Commits ------- b57a83f [Console] Add Lockable trait

xabbuh · 2016-06-23T13:16:23Z

src/Symfony/Component/Console/Command/LockableTrait.php

+     *
+     * @return bool
+     */
+    protected function lock($name = null, $blocking = false)


Why not declaring everything private by default?

protected looks fine to me, moving to private won't provide anything more

Well, every class using this trait will now expose the used methods as an extension point which might not be what the user intended.

xabbuh · 2016-06-23T18:05:54Z

src/Symfony/Component/Console/Command/LockableTrait.php

+/**
+ * Basic lock feature for commands.
+ *
+ * @author Fabien Potencier <fabien@symfony.com>


@geoffrey-brier You should have put your name here. :)

@xabbuh I'm not that important ;)

Of course, you're the author of this trait. :)

…abbuh) This PR was merged into the 3.2-dev branch. Discussion ---------- [Console] LockableTrait: change visibility to private | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18471 (comment) | License | MIT | Doc PR | Without this change we force users to expose unnecessary extension points they may not want to provide. Commits ------- eaa3bb0 LockableTrait: change visibility to private

This PR was merged into the 3.2-dev branch. Discussion ---------- [Console] update the author of the LockableTrait | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #18471 (comment) | License | MIT | Doc PR | Commits ------- b15fec7 update the author of the LockableTrait

carsonbot added the Status: Needs Review label Apr 7, 2016

geoffrey-brier force-pushed the feature/auto-lockable-commands branch 2 times, most recently from f9d7d98 to 3f9fc8a Compare April 7, 2016 11:02

linaori reviewed Apr 7, 2016
View reviewed changes

javiereguiluz added Feature Console labels Apr 7, 2016

xabbuh added Status: Needs Work and removed Status: Needs Review labels Apr 10, 2016

geoffrey-brier force-pushed the feature/auto-lockable-commands branch 2 times, most recently from f5e8f4c to ca06975 Compare June 14, 2016 09:58

fabpot reviewed Jun 23, 2016
View reviewed changes

geoffrey-brier force-pushed the feature/auto-lockable-commands branch 2 times, most recently from b545b81 to 8b63ed3 Compare June 23, 2016 09:32

1ed reviewed Jun 23, 2016
View reviewed changes

nicolas-grekas reviewed Jun 23, 2016
View reviewed changes

geoffrey-brier force-pushed the feature/auto-lockable-commands branch from 8b63ed3 to dd523cb Compare June 23, 2016 11:26

geoffrey-brier changed the title ~~[Console] Add lock feature~~ [Console] Add Lockable trait Jun 23, 2016

[Console] Add Lockable trait

b57a83f

geoffrey-brier force-pushed the feature/auto-lockable-commands branch from dd523cb to b57a83f Compare June 23, 2016 11:42

fabpot merged commit b57a83f into symfony:master Jun 23, 2016

geoffrey-brier deleted the feature/auto-lockable-commands branch June 23, 2016 11:48

xabbuh reviewed Jun 23, 2016
View reviewed changes

xabbuh mentioned this pull request Jun 23, 2016

Document the LockableTrait symfony/symfony-docs#6678

Closed

xabbuh reviewed Jun 23, 2016
View reviewed changes

xabbuh mentioned this pull request Jun 23, 2016

[Console] LockableTrait: change visibility to private #19158

Merged

xabbuh mentioned this pull request Jun 24, 2016

[Console] update the author of the LockableTrait #19164

Merged

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

mohatt mentioned this pull request Nov 30, 2016

Add Lockable trait awesomehub/cli#1

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Console] Add Lockable trait #18471

[Console] Add Lockable trait #18471

geoffrey-brier commented Apr 7, 2016 •

edited

Loading

linaori Apr 7, 2016

javiereguiluz commented Apr 7, 2016

geoffrey-brier commented Apr 7, 2016

linaori commented Apr 7, 2016

geoffrey-brier commented Apr 7, 2016

javiereguiluz commented Apr 7, 2016

linaori commented Apr 7, 2016

geoffrey-brier commented Apr 7, 2016

fabpot commented Apr 28, 2016

lyrixx commented Apr 28, 2016 •

edited

Loading

fabpot commented Jun 14, 2016

geoffrey-brier commented Jun 14, 2016

geoffrey-brier commented Jun 14, 2016 •

edited

Loading

fabpot Jun 23, 2016

lyrixx Jun 23, 2016

fabpot Jun 23, 2016

1ed Jun 23, 2016

geoffrey-brier commented Jun 23, 2016

fabpot commented Jun 23, 2016

1ed Jun 23, 2016

lyrixx commented Jun 23, 2016

nicolas-grekas Jun 23, 2016

geoffrey-brier commented Jun 23, 2016

fabpot commented Jun 23, 2016

geoffrey-brier commented Jun 23, 2016

fabpot commented Jun 23, 2016

xabbuh Jun 23, 2016

lyrixx Jun 23, 2016

nicolas-grekas Jun 23, 2016

xabbuh Jun 23, 2016

xabbuh Jun 23, 2016

geoffrey-brier Jun 23, 2016

xabbuh Jun 24, 2016

[Console] Add Lockable trait #18471

[Console] Add Lockable trait #18471

Conversation

geoffrey-brier commented Apr 7, 2016 • edited Loading

Choose a reason for hiding this comment

javiereguiluz commented Apr 7, 2016

geoffrey-brier commented Apr 7, 2016

linaori commented Apr 7, 2016

geoffrey-brier commented Apr 7, 2016

javiereguiluz commented Apr 7, 2016

linaori commented Apr 7, 2016

geoffrey-brier commented Apr 7, 2016

fabpot commented Apr 28, 2016

lyrixx commented Apr 28, 2016 • edited Loading

fabpot commented Jun 14, 2016

geoffrey-brier commented Jun 14, 2016

geoffrey-brier commented Jun 14, 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

geoffrey-brier commented Jun 23, 2016

fabpot commented Jun 23, 2016

Choose a reason for hiding this comment

lyrixx commented Jun 23, 2016

Choose a reason for hiding this comment

geoffrey-brier commented Jun 23, 2016

fabpot commented Jun 23, 2016

geoffrey-brier commented Jun 23, 2016

fabpot commented Jun 23, 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

Choose a reason for hiding this comment

geoffrey-brier commented Apr 7, 2016 •

edited

Loading

lyrixx commented Apr 28, 2016 •

edited

Loading

geoffrey-brier commented Jun 14, 2016 •

edited

Loading