[Filesystem] Added a LockHandler #10475

lyrixx · 2014-03-17T23:53:28Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#9357 , #3586
License	MIT
Doc PR	https://github.com/symfony/symfony-docs/pull/3956/files

Code sample:

    /**
     * {@inheritdoc}
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $lockHelper = new LockHandler('/tmp/acme/hello.lock');
        if (!$lockHelper->lock()) {
            $output->writeln('The command is already running in another process.');

            return 0;
        }

        $output->writeln(sprintf('Hello <comment>%s</comment>!', $input->getArgument('who')));

        for (;;) {
        }

        $output->writeln(sprintf('bye <comment>%s</comment>!', $input->getArgument('who')));

        $lockHelper->unlock();
    }

cordoval · 2014-03-18T00:22:28Z

src/Symfony/Component/Console/Tests/Locker/FilesystemLockerTest.php

+
+        $this->assertTrue($fl->isLocked('symfony-test-filesystem-lock'));
+    }
+


lyrixx · 2014-03-18T00:41:14Z

Actually, register_shutdown_function is resistant to fatal error. So the PID check is a bonus for linux user.

But signals don't not trigger it. Same thing for __destruct.

cordoval · 2014-03-18T02:46:26Z

please add a checkbox for writing the documentation PR

cordoval · 2014-03-18T04:45:12Z

src/Symfony/Component/Console/Locker/FilesystemLocker.php

+    {
+        file_put_contents($this->getLockFile($name), getmypid());
+    }
+


missing inheritdoc doc blocks?

cordoval · 2014-03-18T05:05:53Z

should we add a soft suggest on the process component? Of course redis sounds like a good fit, about the drivers would be good to first get the use case right, so could you please provide an explanation like @webmozart does ? 👶

lyrixx · 2014-03-18T09:35:36Z

@cordoval The use case is already described in the 2 linked issues.

malarzm · 2014-03-18T10:54:19Z

I think that all drivers will check for pid - isn't it better to have class AbstractLocker which could look like

abstract class AbstractLocker
{
    public function isLocked($name)
    {
        $pid=$this->retrievePid($name);
        // system based check goes here
    }

    abstract function lock($name) {  }

    abstract function unlock($name) {  }

    abstract function retrievePid($name) {  }
}

It should make all drivers more DRY :)

lyrixx · 2014-03-18T11:06:55Z

@malarzm Retrieving the PID is only possible when everything is on the same machine. So it does not make sens. More over, I think I will remove this implementation and use flock

malarzm · 2014-03-18T11:15:46Z

I thought retrievePid would be less confusing than getPid ;) I meant use of $pid = file_get_contents($lockFile); for FilesystemLocker in retrievePid function

stof · 2014-03-18T12:37:08Z

@malarzm what @lyrixx meant is that a locking mechanism which supports locking accross several servers (when you have 2 workers for instance) would not be based on checking a PID.

malarzm · 2014-03-18T12:40:36Z

Ah right, sorry, I think I need more sleep

cordoval · 2014-03-18T13:43:26Z

#3586 talks about a doctrine driver so i guess that is a request.

lyrixx · 2014-03-18T17:13:52Z

Actually, I will remove the interface. And so I will not implement doctrine or redis. Distributed lock is really too hard, with too many edge case. So We will keep something very simple, that work only for one server.

( @romainneutron found this gist for windows)

staabm · 2014-03-18T19:50:41Z

src/Symfony/Component/Console/Locker/FilesystemLocker.php

+
+    public function lock($name)
+    {
+        file_put_contents($this->getLockFile($name), getmypid());


Any error handling? What about permission issues ?

already checked in the constructor.

cordoval · 2014-03-18T19:54:10Z

should we add a soft suggest on the process component?

staabm · 2014-03-18T19:54:14Z

src/Symfony/Component/Console/CHANGELOG.md

@@ -4,6 +4,7 @@ CHANGELOG
 2.5.0
 -----

+* added a way to lock a command


Maybe name it a bit more generic..? It currently allows to lock anything with a given name... Mentioning command here could make users feel it only works with ConsoleCommands or things like that..

jakzal · 2014-03-18T21:48:06Z

@lyrixx keep the interface maybe, to let others provide their own implementations (not in the core)?

romainneutron · 2014-03-18T21:54:11Z

src/Symfony/Component/Console/Locker/FilesystemLocker.php

+        $pid = file_get_contents($lockFile);
+
+        if (class_exists('Symfony\Component\Process\Process')) {
+            $process = new Process('ps -e | awk \'{print $1}\'');


you could use option -p : sprintf('ps -p %d', $pid)

or kill -0 $pid and check the exitcode. This could be done with posix_kill is this function is available

lyrixx · 2014-05-28T09:25:44Z

Hello. I have updated the code. All code came from @romainneutron ;)

Now, we support only one kind of driver (file-system), So there is no need for an interface. It is done on purpose. Using a lock feature in a network is a nightmare. So we do not want to support it at all.

ping @dunglas

staabm · 2014-05-28T09:30:46Z

src/Symfony/Component/Console/Helper/LockHelper.php

+            return true;
+        }
+
+        flock($handle, LOCK_UN);


Success of flock is shown by return value and therefore should be checked against?

not here. See code above... Here we release the lock we get.

But the release could also fail, couldn't it?

I don't think so.

dunglas · 2014-05-28T17:09:55Z

I've some concerns about this implementation:

it doesn't fit my use case. I need blocking locks to allow a command waiting the end of a previous one before running. Here, the non blocking mode is hardcoded (and according to the PHP doc, this mode is not supported on Windows)
I think it's not the responsibility of this class to kill processes. Let the developer - or the OS - doing that. And what if the killed process is a daemon or something like that? Locks aren't necessary pid files.
The interface was a good idea IMO (and I'll probably need "network" locks soon). A memcache lock should not be too hard to implement (even outside of core as already said).

What about

interface Lock
{
    public function lock($blocking = true);
    public function unlock();
    public function isLocked();
}

lyrixx · 2014-05-28T17:36:21Z

it doesn't fit my use case. I need blocking locks to allow a command waiting the end of a previous one before running. Here, the non blocking mode is hardcoded (and according to the PHP doc, this mode is not supported on Windows)

Why not ;)

I think it's not the responsibility of this class to kill processes. Let the developer - or the OS - doing that. And what if the killed process is a daemon or something like that? Locks aren't necessary pid files.

We kill nothing.

The interface was a good idea IMO (and I'll probably need "network" locks soon). A memcache lock should not be too hard to implement (even outside of core as already said).

Actually, It's VERY VERY hard to implement. What if your command segfault ? nobody will release the lock... The only almost working implementation implies heart beat... And this is hard to implements. See the "warning" section I added to the LockHelper class

Then, If you need a custom lockHelper, just create a new one. There is no need to use an interface for that ;)

dunglas · 2014-05-28T18:13:53Z

src/Symfony/Component/Console/Helper/LockHelper.php

+
+namespace Symfony\Component\Console\Helper;
+
+use Symfony\Component\Console\Locker\LockerInterface;


If no interface, this statement must me removed.

lyrixx · 2014-09-11T21:18:54Z

@Tobion Thanks for the review. I hope it's ok now ;)

Tobion · 2014-09-11T22:14:13Z

src/Symfony/Component/Filesystem/LockHandler.php

+
+        $lockPath = $lockPath ?: sys_get_temp_dir();
+
+        $file = sprintf('%s/%s', $lockPath, $name);


local variable $file is not necessary. just use $this->file

lyrixx · 2014-09-22T08:47:27Z

Travis and fabbot are of. shippable no and I don't know why. ping @fabpot.

fabpot · 2014-09-22T08:52:11Z

forget about shippable, it was just a test

lyrixx · 2014-09-22T09:24:19Z

So it's ready for merge

fabpot · 2014-09-22T10:14:05Z

👍

rybakit · 2014-09-22T11:34:18Z

Just curious what you folks think about getting rid of the file locks and using port bindings, like it's described here?

fabpot · 2014-09-22T12:38:07Z

Thank you @lyrixx.

@inheritdoc

This PR was merged into the 2.6-dev branch. Discussion ---------- [Filesystem] Added a LockHandler | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #9357 , #3586 | License | MIT | Doc PR | https://github.com/symfony/symfony-docs/pull/3956/files Code sample: ```php /** * {@inheritdoc} */ protected function execute(InputInterface $input, OutputInterface $output) { $lockHelper = new LockHandler('/tmp/acme/hello.lock'); if (!$lockHelper->lock()) { $output->writeln('The command is already running in another process.'); return 0; } $output->writeln(sprintf('Hello <comment>%s</comment>!', $input->getArgument('who'))); for (;;) { } $output->writeln(sprintf('bye <comment>%s</comment>!', $input->getArgument('who'))); $lockHelper->unlock(); } ``` ![process-lock](https://f.cloud.github.com/assets/408368/2443205/4f0bf3e8-ae30-11e3-9bd4-78e09e2973ad.png) Commits ------- 9ad8957 [Filesystem] Added a lock handler

lyrixx · 2014-09-22T12:39:33Z

@rybakit your idea is interesting, but I'm not fan of it because

the port could be already used by another application on production server (and not on your local machine)
I don't know if it work well on windows
It takes a port.
It's a bit a hack, port did not designed for that, where flock are.

iainp999 · 2014-09-26T09:40:45Z

Hi guys. I have a similar project already (https://github.com/iainp999/lockman - approach is slightly different, happy for feedback) but would be interested in contributing to a more generic solution for SF2.

stof · 2014-09-26T13:47:10Z

@iainp999 We explicitly rejected adding distributed locking in core. People needing such feature would then need to use a library providing such feature (either your library, or one of the others available on Packagist)

iainp999 · 2014-09-26T14:26:30Z

@stof oh ok, cool

This PR was merged into the master branch. Discussion ---------- [Command] Added LockHelper | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes symfony/symfony#10475 | Applies to | Symfony version 2.6+ | Fixed tickets | - Commits ------- 909a294 [Filesystem] Added documentation for the new LockHandler 2bf8c55 [Filesystem] Moved current document to a dedicated folder

temple · 2017-04-03T10:33:11Z

Could you please @lyrixx provide an interface for the LockHandler class?
It would be useful to enhance this class by using wrappers.. but this means having an interface to implement.

lyrixx · 2017-04-03T11:11:43Z

@temple We do not provide an interface on purpose. It works only on one single server.

If you want something more sophisticated, please use the Lock component instead.

temple · 2017-04-03T12:07:54Z

Are you referring this one, @lyrixx ?
http://symfony.com/blog/new-in-symfony-3-3-lock-component

lyrixx · 2017-04-03T12:24:58Z

yes.

temple · 2017-04-03T13:11:56Z

Thank you very much!

cordoval reviewed Mar 18, 2014
View reviewed changes

staabm reviewed Mar 18, 2014
View reviewed changes

romainneutron reviewed Mar 18, 2014
View reviewed changes

lyrixx added the Console label Mar 26, 2014

lyrixx changed the title ~~[WIP][Command] Added an helper to lock a command~~ [Command] Added an helper to lock a command May 28, 2014

staabm reviewed May 28, 2014
View reviewed changes

dunglas reviewed May 28, 2014
View reviewed changes

lyrixx force-pushed the command-lock branch 2 times, most recently from 0201f9f to a7aabd2 Compare September 11, 2014 21:17

Tobion reviewed Sep 11, 2014
View reviewed changes

[Filesystem] Added a lock handler

9ad8957

lyrixx force-pushed the command-lock branch from a7aabd2 to 9ad8957 Compare September 12, 2014 14:20

fabpot merged commit 9ad8957 into symfony:master Sep 22, 2014

GDmac mentioned this pull request Nov 10, 2014

Regarding Lockhandler #12447

Closed

lyrixx deleted the command-lock branch December 8, 2015 15:21

kidol mentioned this pull request Oct 27, 2016

FileMutex: unlink(...): No such file or directory yiisoft/yii2#12860

Closed


		$this->assertTrue($fl->isLocked('symfony-test-filesystem-lock'));
		}


		namespace Symfony\Component\Console\Helper;

		use Symfony\Component\Console\Locker\LockerInterface;


		$lockPath = $lockPath ?: sys_get_temp_dir();

		$file = sprintf('%s/%s', $lockPath, $name);

[Filesystem] Added a LockHandler #10475

[Filesystem] Added a LockHandler #10475

Conversation

lyrixx commented Mar 17, 2014

Choose a reason for hiding this comment

lyrixx commented Mar 18, 2014

cordoval commented Mar 18, 2014

Choose a reason for hiding this comment

cordoval commented Mar 18, 2014

lyrixx commented Mar 18, 2014

malarzm commented Mar 18, 2014

lyrixx commented Mar 18, 2014

malarzm commented Mar 18, 2014

stof commented Mar 18, 2014

malarzm commented Mar 18, 2014

cordoval commented Mar 18, 2014

lyrixx commented Mar 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cordoval commented Mar 18, 2014

Choose a reason for hiding this comment

jakzal commented Mar 18, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lyrixx commented May 28, 2014

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

dunglas commented May 28, 2014

lyrixx commented May 28, 2014

Choose a reason for hiding this comment

lyrixx commented Sep 11, 2014

Choose a reason for hiding this comment

lyrixx commented Sep 22, 2014

fabpot commented Sep 22, 2014

lyrixx commented Sep 22, 2014

fabpot commented Sep 22, 2014

rybakit commented Sep 22, 2014

fabpot commented Sep 22, 2014

lyrixx commented Sep 22, 2014

iainp999 commented Sep 26, 2014

stof commented Sep 26, 2014

iainp999 commented Sep 26, 2014

temple commented Apr 3, 2017

lyrixx commented Apr 3, 2017

temple commented Apr 3, 2017

lyrixx commented Apr 3, 2017

temple commented Apr 3, 2017