[Filesystem] Added a LockHandler #10475

Merged
merged 1 commit into from Sep 22, 2014

Projects

None yet
Member
lyrixx commented Mar 17, 2014
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();
    }

process-lock

@cordoval cordoval commented on an outdated diff Mar 18, 2014
...mponent/Console/Tests/Locker/FilesystemLockerTest.php
+ $fl = new FilesystemLocker($tmp);
+
+ $this->assertFalse($fl->isLocked('symfony-test-filesystem-lock'));
+ }
+
+ public function testIsLockWhenLockedButPidIsAlive()
+ {
+ $tmp = sys_get_temp_dir();
+ $lockFile = "$tmp/symfony-test-filesystem-lock";
+ file_put_contents($lockFile, getmypid());
+
+ $fl = new FilesystemLocker($tmp);
+
+ $this->assertTrue($fl->isLocked('symfony-test-filesystem-lock'));
+ }
+
cordoval
cordoval Mar 18, 2014 Contributor

oh!

Member
lyrixx commented Mar 18, 2014

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.

Contributor

please add a checkbox for writing the documentation PR

@cordoval cordoval commented on an outdated diff Mar 18, 2014
...Symfony/Component/Console/Locker/FilesystemLocker.php
+ throw new \RuntimeException(sprintf("Process %s did not finished correctly.\nSTDOUT:\n%s\nSTDERR:\n%s\n", $process->getCommandLine(), $output, $process->getErrorOutput()));
+ }
+
+ $pids = $process->getOutput();
+ } else {
+ $pids = `ps -e | awk '{print $1}'`;
+ }
+
+ return in_array($pid, explode("\n", $pids));
+ }
+
+ public function lock($name)
+ {
+ file_put_contents($this->getLockFile($name), getmypid());
+ }
+
cordoval
cordoval Mar 18, 2014 Contributor

missing inheritdoc doc blocks?

Contributor

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 ? 👶

Member
lyrixx commented Mar 18, 2014

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

Contributor
malarzm commented Mar 18, 2014

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

Member
lyrixx commented Mar 18, 2014

@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

Contributor
malarzm commented Mar 18, 2014

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

Member
stof commented Mar 18, 2014

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

Contributor
malarzm commented Mar 18, 2014

Ah right, sorry, I think I need more sleep

Contributor

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

Member
lyrixx commented Mar 18, 2014

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 staabm and 1 other commented on an outdated diff Mar 18, 2014
...Symfony/Component/Console/Locker/FilesystemLocker.php
+
+ if (!$process->isSuccessful()) {
+ throw new \RuntimeException(sprintf("Process %s did not finished correctly.\nSTDOUT:\n%s\nSTDERR:\n%s\n", $process->getCommandLine(), $output, $process->getErrorOutput()));
+ }
+
+ $pids = $process->getOutput();
+ } else {
+ $pids = `ps -e | awk '{print $1}'`;
+ }
+
+ return in_array($pid, explode("\n", $pids));
+ }
+
+ public function lock($name)
+ {
+ file_put_contents($this->getLockFile($name), getmypid());
staabm
staabm Mar 18, 2014 Contributor

Any error handling? What about permission issues ?

lyrixx
lyrixx Mar 18, 2014 Member

already checked in the constructor.

Contributor

should we add a soft suggest on the process component?

@staabm staabm commented on an outdated diff Mar 18, 2014
src/Symfony/Component/Console/CHANGELOG.md
@@ -4,6 +4,7 @@ CHANGELOG
2.5.0
-----
+* added a way to lock a command
staabm
staabm Mar 18, 2014 Contributor

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

Member
jakzal commented Mar 18, 2014

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

@romainneutron romainneutron commented on an outdated diff Mar 18, 2014
...Symfony/Component/Console/Locker/FilesystemLocker.php
+
+ public function isLocked($name)
+ {
+ $lockFile = $this->getLockFile($name);
+ if (!file_exists($lockFile)) {
+ return false;
+ }
+
+ if (defined('PHP_WINDOWS_VERSION_BUILD')) {
+ return true;
+ }
+
+ $pid = file_get_contents($lockFile);
+
+ if (class_exists('Symfony\Component\Process\Process')) {
+ $process = new Process('ps -e | awk \'{print $1}\'');
romainneutron
romainneutron Mar 18, 2014 Member

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

romainneutron
romainneutron Mar 18, 2014 Member

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

@romainneutron romainneutron commented on an outdated diff Mar 18, 2014
...Symfony/Component/Console/Locker/FilesystemLocker.php
+ if (!is_writable($lockPath)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->lockPath = $lockPath;
+ }
+
+ public function isLocked($name)
+ {
+ $lockFile = $this->getLockFile($name);
+ if (!file_exists($lockFile)) {
+ return false;
+ }
+
+ if (defined('PHP_WINDOWS_VERSION_BUILD')) {
+ return true;
romainneutron
romainneutron Mar 18, 2014 Member

On windows you can detect if a program is running given a PID using the command sprintf('tasklist /FI "PID eq %d"', $pid) and check the exitcode

@lyrixx lyrixx added the Console label Mar 26, 2014
@lyrixx lyrixx changed the title from [WIP][Command] Added an helper to lock a command to [Command] Added an helper to lock a command May 28, 2014
Member
lyrixx commented May 28, 2014

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 staabm and 1 other commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ $handle = @fopen($this->file, 'r');
+
+ // the file does not exist
+ if (!is_resource($handle)) {
+ return false;
+ }
+
+ $locker = true;
+ if (false === flock($handle, LOCK_EX | LOCK_NB, $locker)) {
+ // exclusive lock failed, another process is locking
+ fclose($handle);
+
+ return true;
+ }
+
+ flock($handle, LOCK_UN);
staabm
staabm May 28, 2014 Contributor

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

lyrixx
lyrixx May 28, 2014 Member

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

staabm
staabm May 28, 2014 Contributor

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

lyrixx
lyrixx May 28, 2014 Member

I don't think so.

@nicolas-grekas nicolas-grekas and 1 other commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ }
+
+ public function unlock()
+ {
+ if (is_resource($this->handle)) {
+ flock($this->handle, LOCK_UN | LOCK_NB);
+ ftruncate($this->handle, 0);
+ fclose($this->handle);
+ }
+
+ if (is_file($this->file)) {
+ unlink($this->file);
+ }
+ }
+
+ public function isLocked()
nicolas-grekas
nicolas-grekas May 28, 2014 Owner

Having both isLocked and lock makes the feature non-atomic.
I think it would be better to remove isLocked and keep only lock, making it return true/false (and non blocking as it already is)

lyrixx
lyrixx May 28, 2014 Member

I think I agree ;)

@nicolas-grekas nicolas-grekas commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ throw new \InvalidArgumentException(sprintf('The directory "%s" does not exist and could not be created.', $lockPath));
+ }
+
+ if (!is_writable($lockPath)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->file = $file;
+ }
+
+ public function unlock()
+ {
+ if (is_resource($this->handle)) {
+ flock($this->handle, LOCK_UN | LOCK_NB);
+ ftruncate($this->handle, 0);
+ fclose($this->handle);
nicolas-grekas
nicolas-grekas May 28, 2014 Owner

Add $this->handle = null; ?

@nicolas-grekas nicolas-grekas commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->file = $file;
+ }
+
+ public function unlock()
+ {
+ if (is_resource($this->handle)) {
+ flock($this->handle, LOCK_UN | LOCK_NB);
+ ftruncate($this->handle, 0);
+ fclose($this->handle);
+ }
+
+ if (is_file($this->file)) {
+ unlink($this->file);
nicolas-grekas
nicolas-grekas May 28, 2014 Owner

This creates a race condition. The file should not be removed.

Member
dunglas commented May 28, 2014

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();
}
Member
lyrixx commented May 28, 2014

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 dunglas commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
@@ -0,0 +1,130 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Console\Helper;
+
+use Symfony\Component\Console\Locker\LockerInterface;
dunglas
dunglas May 28, 2014 Member

If no interface, this statement must me removed.

@dunglas dunglas commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+
+ public function unlock()
+ {
+ if (is_resource($this->handle)) {
+ flock($this->handle, LOCK_UN | LOCK_NB);
+ ftruncate($this->handle, 0);
+ fclose($this->handle);
+ $this->handle = null;
+ }
+
+ if (is_file($this->file)) {
+ unlink($this->file);
+ }
+ }
+
+ public function lock()
dunglas
dunglas May 28, 2014 Member

With a $blockingparameter the blocking mode can be easily supported (only the bitmask must be adapted depending the value of the variable).

@dunglas dunglas commented on an outdated diff May 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ if (false === flock($handle, LOCK_EX | LOCK_NB, $locker)) {
+ // exclusive lock failed, another process is locking
+ fclose($handle);
+
+ return true;
+ }
+
+ flock($handle, LOCK_UN);
+ fclose($handle);
+
+ // lock succeed, anyway, some systems may not support this very well,
+ // let's check for a Pid running
+ $pid = file_get_contents($this->file);
+
+ if (function_exists('posix_kill')) {
+ $running = posix_kill($pid, 0);
dunglas
dunglas May 28, 2014 Member

This trick seems to be undocumented (http://php.net/posix_kill and man signal on a Mac) and can return false even if the process is running (if the current process doesn't have the right to send signals).

Member
dunglas commented May 28, 2014

We kill nothing.

Sorry I was on my phone and I missed the meaning of this code snippet.

Actually, It's VERY VERY hard to implement.

I agree, but implementations exist anyway (see below).

What if your command segfault ?

What if a command (or any process) having a filesystem lock segfault? The OS can take minutes to release the lock. The lock can even be set "forever".

IMO this is not the responsibility of a standard lock system in a web framework to handle failures. This can be the responsibility of a monitoring system, or of custom implementations.
Depending of the use case, heartbeats - like you suggest -, locks that automatically expire after a reasonable time (think memcache), tests to see if the job is still processed regularly or anything else can be used.

Description of a use case for the interface:
Two commands must never run the same time. I create a superclass, say "NotInTheSameTimeCommand" that use a lock (provided by dependency injection):

// Sample code
// use ...

abstract class NotInTheSameTimeCommand extends ContainerAwareCommand
{
   private $locker;

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

   public function run(InputInterface $input, OutputInterface $output)
   {
       $lock->lock(); // Blocking mode
       $code = parent:run($input, $output);
       $lock->unlock();

       return $code;
   }
}

If the commands are on the same machine, it will work fine. But what if a customer wants to run my app on a distributed env (commands run on separates servers)? With an interface I "just" (yes, this is not trivial) inject a "distributed" locker.
Thanks to the abstraction I can use Memcache, Redis, or anything that fit my use case (a remote webservice?).

But for now, my use case is just the local blocking mode :)

Member
lyrixx commented May 29, 2014

I updated the code to use only flock

@dunglas: I understand an interface could fit. But this adds not value here.

Member
lyrixx commented May 29, 2014

@dunglas I added a blocking option.

Owner

👍
Looks like you can tick the checkboxes in your introduction.

Member
lyrixx commented May 29, 2014

@nicolas-grekas no, I did not open a PR on the doc repo yet ;)

Member
dunglas commented May 29, 2014

👍
Good job!

@nicolas-grekas nicolas-grekas and 1 other commented on an outdated diff May 29, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ }
+
+ if (!is_writable($lockPath)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->file = $file;
+ }
+
+ public function lock($blocking = false)
+ {
+ if ($this->handle) {
+ return true;
+ }
+
+ // Set an error handle to not trigger the registered error handler.
nicolas-grekas
nicolas-grekas May 29, 2014 Owner

Missing r: handler

@staabm staabm and 4 others commented on an outdated diff May 30, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ */
+
+namespace Symfony\Component\Console\Helper;
+
+/**
+ * LockHelper class provides helper to allow only once instance of a command to
+ * run.
+ *
+ * WARNING: This helper works only when using one and only one host. If you have
+ * several hosts, you must not use this helper.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHelper
staabm
staabm May 30, 2014 Contributor

would a name like FileLock be more precise here?

lyrixx
lyrixx May 30, 2014 Member

May be. I don't know. This name is explicit enough, and smaller ;)

dunglas
dunglas May 30, 2014 Member

This class can be useful in a lot of contexts (even in a web context) and is not only useful for "Console".
Maybe can it be moved in the Processor Filesystem component?

henrikbjorn
henrikbjorn May 30, 2014 Contributor

Would have to agree about moving it, and the name should be changed to "LockFile" which is what it is.

lyrixx
lyrixx May 30, 2014 Member

LockFile could mean: "We try to lock the read / write of a file" which is not the case here.

staabm
staabm May 30, 2014 Contributor

FileMutex?

KingCrunch
KingCrunch Jun 18, 2014 Contributor

LockFile with acquire and release? That could also base on an interface

interface Lock {
    public function acquire();
    public function release();
}

or even Semaphore

interface Semaphore {
    public function acquire($count = 1);
    public function release($count = 1);
}
Member
dunglas commented May 30, 2014

@lyrixx Added a test for the blocking mode: lyrixx#5

Member
dunglas commented May 30, 2014

A working snippet to create commands that wait for the previous ones to finish before running:

<?php

namespace Dunglas\FrameworkBundle\Command;

use Dunglas\FrameworkBundle\Helper\LockHelper;
use Symfony\Bundle\FrameworkBundle\Command\ContainerAwareCommand;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;

/**
 * Uses a lock to allow only one command to run at the same time.
 *
 * @author Kévin Dunglas <dunglas@gmail.com>
 */
abstract class LockCommand extends ContainerAwareCommand
{
    /**
     * {@inheritdoc}
     */
    public function run(InputInterface $input, OutputInterface $output)
    {
        $lock = new LockHelper($this->getContainer()->getParameter('kernel.cache_dir') . '/command.lock');
        $lock->lock(true);

        try {
            return parent::run($input, $output);
        } finally {
            $lock->unlock();
        }
    }

}
Member

👍 Please open a doc PR and I think it's alright

Member

@lyrixx Please rebase

@lyrixx lyrixx referenced this pull request in symfony/symfony-docs Jun 17, 2014
Merged

[Command] Added LockHelper #3956

Member
lyrixx commented Jun 17, 2014

Rebased, doc PR opened.

Contributor

update table PR field and also todo checkbox list please

Member
lyrixx commented Jun 17, 2014

@cordoval fixed.

@cordoval cordoval commented on an outdated diff Jun 17, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
@@ -0,0 +1,81 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Console\Helper;
+
+/**
+ * LockHelper class provides helper to allow only once instance of a command to
cordoval
cordoval Jun 17, 2014 Contributor

provides a helper, or rather provides a way to allow ... ?

Member
lyrixx commented Jun 24, 2014

@fabpot This PR is ready for a final review and / or merge

Member

What about registering the helper in the Console\Application helper set?

Member
lyrixx commented Jun 24, 2014

Because you have to give an arg to the constructor ?

Member

absolutely, that's fine.

Member

👍

Member
Tobion commented Jun 28, 2014

I don't see what a this lock helper has to do with the console component. It could be used anywhere.
So IMO it doesn't belong here and could just be a new external bundle. I'm 👎 at the moment.

Member
Tobion commented Jun 28, 2014

Btw, locking could also be done using databases etc. See #10931 So maybe it could be a new component with different implemenations. But I think it's kinda difficult to make a useable component out of it.

Btw nr. 2, usually locks also block for a given time. This does not seem the case here.

Member
lyrixx commented Jun 28, 2014

What is nr. 2 ?

Member
Tobion commented Jun 28, 2014

number 2

Member
lyrixx commented Jun 28, 2014

Btw, locking could also be done using databases etc.

Yes, you can implement lot of way to lock a command, but with this implementation there is no external dependency. It does the job. Therefore there is no other advantage to use a DB / Redis / .. to lock a command when you use a single server. As We said before, Locking when there are multiple servers is too complicated, and need external tools. So we are not going to implement these solutions here.

I don't see what a this lock helper has to do with the console component. It could be used anywhere. So IMO it doesn't belong here and could just be a new external bundle

Yes, you can find use case for "lock" outside the console component. For instance people used to use lock with DB or redis when it is related to data. Here we are not talking about data. It's just for workflow. But I totally understand that people may want to use this helper to lock data process. And I think it's a bad idea because it's not salable. It works with only one server; So it should not be done like that.

Here, it's just a simple command helper, with known limitation (works with one and only server), only for the console component.

A generic solution for lock is just impossible. Please do not block this PR because you want something that is impossible in the core ;)

Then why do you want to create a new bundle ? It will not work with silex. I prefer to keep thing as simple as possible.

Finally, what does "number 2" refer ?

@staabm staabm and 1 other commented on an outdated diff Jun 28, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Console\Helper;
+
+/**
+ * LockHelper class provides helper to allow only once instance of a command to
+ * run.
+ *
+ * WARNING: This helper works only when using one and only one host. If you have
+ * several hosts, you must not use this helper.
staabm
staabm Jun 28, 2014 Contributor

@lyrixx since this limitation is rather important you might describe the reasons with more than one sentence?

lyrixx
lyrixx Jun 28, 2014 Member

Yes, why not ;)
I'm not a native English speaker, Do you have a good sentence ?

Member
Tobion commented Jun 28, 2014

Number 2 means, it's the second "btw".
My point is, this lock helper is not tied to the console component and totally unrelated class. Just make a new repository with it. It doesn't have to be a "bundle". Of course it will work with Silex then as well.

Member
lyrixx commented Jun 28, 2014

new repository, like a new component? Having this class in the core it's easier for me, and I guess much more people. ;)

About the blocking timing: It's up to you. You can ask to wait the release of the lock, or not.

@jderusse jderusse and 2 others commented on an outdated diff Jul 3, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ // On Windows, even if PHP doc says the contrary, LOCK_NB works, see
+ // https://bugs.php.net/54129
+ if (!flock($this->handle, LOCK_EX | ($blocking ? 0 : LOCK_NB))) {
+ fclose($this->handle);
+ $this->handle = null;
+
+ return false;
+ }
+
+ return true;
+ }
+
+ public function unlock()
+ {
+ if ($this->handle) {
+ flock($this->handle, LOCK_UN | LOCK_NB);
jderusse
jderusse Jul 3, 2014 Contributor

What is the purpose of the LOCK_NB when you release an exclusive lock ?

lyrixx
lyrixx Jul 3, 2014 Member

from http://fr2.php.net/flock

It is also possible to add LOCK_NB as a bitmask to one of the above operations if you don't want flock() to block while locking. (not supported on Windows)

PHP supports a portable way of locking complete files in an advisory way (which means all accessing programs have to use the same way of locking or it will not work). By default, this function will block until the requested lock is acquired; this may be controlled (on non-Windows platforms) with the LOCK_NB option documented below.

So you think it is useless here ?

staabm
staabm Jul 3, 2014 Contributor

You are relying on the result of flock() in this lines, so make it non-blocking would break the logic, doesn't it?
Having it non-blocking would only make sense if you not immediately fclose() afterwards (but do the flock "sometime later")

jderusse
jderusse Jul 3, 2014 Contributor

It was just a question because I don't see the use case.
If the unlock was blocking, in which case the process will be paused?

BTW, it's costless.

Member

@Tobion Such lock file feature would be very useful, for example in #11311.

Member
lyrixx commented Aug 1, 2014

So, should I move this class to the FS component? I have no thought about that.

But I disagree the move to the Process component. This class is not related to process.

Should it be a new component? Lock? It's weird for me to have a component with only one class. But it's already the case for the FS component.
Anyway, I think it is a bad idea to create a new Lock Component because people will want to implement more locking strategy (redis, memcache, db...). And it's really hard to do it well. So IMHO, we should no promote that.

Owner

I agree with putting this in the fs component. It's coherent with the locking strategy and says in the name that no other locking strategy is expected to be build later, which is safe, because locking with the network in between is way more complex

Contributor
jderusse commented Aug 1, 2014

@lyrixx What if someone do a well implementation of dbLock ? It can be realy usefull for project using multiple servers

FileSystem is not the target of your class, it's a dependency used by your implementation.

Member
lyrixx commented Aug 1, 2014

What if someone do a well implementation of dbLock

After a talk few weeks / month ago with @fabpot , we did not want this kind of code in symfony, because it's too hard to do something very strong. (how to release a lot when PHP die after a fatal error, or after a segfault ?)

FileSystem is not the target of your class, it's a dependency used by your implementation.

Does that mean you don't want the move to FS component? What do you propose instead?

Contributor
jderusse commented Aug 1, 2014

When a process terminate, File System release his exclusives locks, StreamSockets are closed, SGDB rollbacks his pending transactions. I thinks there is a place for extending properly your implementation.

BTW, actualy there is no component who fits with this class. If this feature would be shared with other bundles/component, I think it should have is own component. Does it's worst it ? #10962 (comment)

Contributor
staabm commented Aug 7, 2014

Just came into the situation that I would require such a LOCK component :-/

@staabm staabm and 2 others commented on an outdated diff Aug 7, 2014
src/Symfony/Component/Console/Helper/LockHelper.php
+ restore_error_handler();
+
+ if (!$this->handle) {
+ throw new \RuntimeException(sprintf('Unable to fopen "%s".', $this->file));
+ }
+
+ // On Windows, even if PHP doc says the contrary, LOCK_NB works, see
+ // https://bugs.php.net/54129
+ if (!flock($this->handle, LOCK_EX | ($blocking ? 0 : LOCK_NB))) {
+ fclose($this->handle);
+ $this->handle = null;
+
+ return false;
+ }
+
+ return true;
staabm
staabm Aug 7, 2014 Contributor

After successfull locking the unlock method should be registered as a shutdown function (to release the lock on fatal errors) and also on pcntl_signal(SIGINT, ...) to cleanup the lock when a CLI process is interrupted using CTRL+C (this will only work on *nix though).

lyrixx
lyrixx Aug 7, 2014 Member

it's already handled by PHP itself.

staabm
staabm Aug 7, 2014 Contributor

ah right... I am used to real files for locks, but since you are using flock this is not a problem

staabm
staabm Aug 7, 2014 Contributor

but you are creating the file for locking... you should clean it up to not reach max files problems, shouldn't you (cleaning up the file is not required on fatal erros etc. but it should be done on successfull unlock)?

lyrixx
lyrixx Aug 7, 2014 Member

you should clean it up to not reach max files problems

We are using the same file over and over. So we are not going to reach max file issue.

cleaning up the file is not required on fatal erros etc. but it should be done on successfull unlock

Yes, I will do that.

xabbuh
xabbuh Aug 7, 2014 Member

We are using the same file over and over. So we are not going to reach max file issue.

But the file name is passed to the constructor. So, if you have many calls of it each with different files, that might become an issue.

staabm
staabm Aug 7, 2014 Contributor

PR see lyrixx#6

lyrixx
lyrixx Aug 7, 2014 Member

Yes, As said, I will add the unlink ;).

But the file name is passed to the constructor. So, if you have many calls of it each with different files, that might become an issue.

But IIUC, this is a false-issue. How many commands that needs lock do we have? one to ten ? even with one hundred commands it is not an issue.

But, after the move to the FS component, people could use it a lot for everything ;) This is why I will unset the file ;)

Contributor
staabm commented Aug 7, 2014

what to you think about a shorthand method with a callable?

$locker = new LockHelper();
$locker->mutualAccess(function() {
  // do whatever you like, LockHelper will do the locking+unlocking 
  // for you before/after this callable is executed
});

I am not sold on the method name...

Member
lyrixx commented Aug 7, 2014

what to you think about a shorthand method with a callable?

I'm not sure to like it because:

  • you have to put lot of code inside the closure
  • not easy to read
  • do not respect the fail fast principal
  • you do not know if the code was executed or not (could be fixed, but the code will become ugly)
@lyrixx lyrixx changed the title from [Command] Added an helper to lock a command to [Filesystem] Added a LockHandler Aug 7, 2014
Member
lyrixx commented Aug 7, 2014

I moved the class to the Filesystem Component. And now the unlock remove the file from the filesystem.

@staabm staabm and 1 other commented on an outdated diff Aug 7, 2014
src/Symfony/Component/Filesystem/LockHandler.php
@@ -0,0 +1,82 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Filesystem;
+
+/**
+ * LockHelper class provides helper to allow only once instance of a command to
staabm
staabm Aug 7, 2014 Contributor

LockHelper->LockHandler

lyrixx
lyrixx Aug 7, 2014 Member

thanks, fixed

Member
lyrixx commented Aug 7, 2014

Acutally, I remove the unlink, because it does not work (race condition):

A and B are two process.

  1. A lock
  2. A unlock (start)
  3. A unlock => fclock(unlock)
  4. B lock
  5. A unlock => unlink

Now B use a lock file that does not exist on the FS anymore.

Member
Tobion commented Aug 7, 2014

When working with locks you always have to implement retry-logic because you can never 100% prevent race conditions.

Btw, as far as I see most deciders already expressed their -1 for locking
stof: #9357 (comment)
fabpot: #3586 (comment)
tobion: #10475 (comment)

Member
Tobion commented Aug 7, 2014

One more thing. From an API point of view (imagine a LockInterface), you would want to express what thing to lock (e.g. command name), e.g. function lock($name) and based on that the file name is determined. Not the other way round.

@Taluu Taluu commented on an outdated diff Aug 7, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ public function __construct($file)
+ {
+ $lockPath = dirname($file);
+
+ if (!is_dir($lockPath) && !@mkdir($lockPath, 0777, true)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" does not exist and could not be created.', $lockPath));
+ }
+
+ if (!is_writable($lockPath)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
Taluu
Taluu Aug 7, 2014 Contributor

Shouldn't you use the Filesystem class here ?

Member
lyrixx commented Aug 14, 2014

One more thing. From an API point of view (imagine a LockInterface), you would want to express what thing to lock (e.g. command name), e.g. function lock($name) and based on that the file name is determined. Not the other way round.

  1. I had an initial use case: lock command to allow only one instance at the same time
  2. So I create a PR with a command lock helper with an interface.
  3. we talked with @fabpot @nicolas-grekas @romainneutron (offline), and we decided to not add an interface, because only the FS Lock is easy to maintain / implement and is enough robust.
  4. Then some people here said I should not put this Helper into the Command NS, So I put it in the FS NS.

So now, I do not understand what is happening. What do you want?

Then @fabpot could be 👍 with this PR iff there is no interface :) And I did not know for @stof

Member
stof commented Sep 4, 2014

👍

@nicolas-grekas nicolas-grekas and 2 others commented on an outdated diff Sep 4, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * LockHandler class provides a simple abstraction to work with file lock
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ public function __construct($file)
+ {
+ $lockPath = dirname($file);
+
+ if (!is_dir($lockPath) && !@mkdir($lockPath, 0777, true)) {
nicolas-grekas
nicolas-grekas Sep 4, 2014 Owner

Could/should this benefit from work done in #11726 (mkdir race condition)?

lyrixx
lyrixx Sep 4, 2014 Member

Yes, sure. Moreover, this class is now in the same component ;) do that right now ;)

Tobion
Tobion Sep 9, 2014 Member

Also wouldn't it be better to just do stuff like creating a directory when actually used, i.e. when calling lock()?
This way it can also be used as a service without overhead.

Member
lyrixx commented Sep 8, 2014

@Tobion You are the only last one against this PR. What should I do to make this PR mergeable?

Member
stof commented Sep 9, 2014

@Tobion ping

@Tobion Tobion commented on an outdated diff Sep 9, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+
+ if (!is_writable($lockPath)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->file = $file;
+ }
+
+ public function lock($blocking = false)
+ {
+ if ($this->handle) {
+ return true;
+ }
+
+ // Set an error handler to not trigger the registered error handler if
+ // the file can not be open.
Tobion
Tobion Sep 9, 2014 Member

typo: opened

@Tobion Tobion commented on an outdated diff Sep 9, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ public function __construct($file)
+ {
+ $lockPath = dirname($file);
+
+ if (!is_dir($lockPath) && !@mkdir($lockPath, 0777, true)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" does not exist and could not be created.', $lockPath));
+ }
+
+ if (!is_writable($lockPath)) {
+ throw new \InvalidArgumentException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->file = $file;
+ }
+
+ public function lock($blocking = false)
Tobion
Tobion Sep 9, 2014 Member

missing phpdocs on all methods

Member
Tobion commented Sep 9, 2014

How is ensured the lock is released at script end?

Member
Tobion commented Sep 9, 2014

So if I want to actually write to the locked file, I would need to create another file handle? Because I cannot access to file handle created by the lock handler.

Contributor
Nicofuma commented Sep 9, 2014

@Tobion Worse, the class like that can't be used at all because you need to call flock yourself.

Contributor
Nicofuma commented Sep 9, 2014

For your first question the lock is released at script end by the system because a new process is created, if you use php with FPM it should be the same I think but I'm not sure (it could depends on the OS and the strategy with the locks acquired by sub processes... but in general they should be released at the end of the sub process) and with HHVM I don't know at all.

Member
stof commented Sep 10, 2014

@Tobion what is your use case for writing in the lock file ?

Member
Tobion commented Sep 10, 2014

Well the main point of locking a file is that only one process can write to it.

Member
stof commented Sep 10, 2014

The sue case of the LockHandler is not locking a file (for this, PHP has the methods already in its file API). The use case is locking your command, to forbid running it several times concurrently. and for this, it relies on a file lock internally (you could implement it using a lock stored in Redis rather than on the filesystem but it gets much more complex, especially to ensure they are released on fatal errors, and we voted against adding such behavior in Symfony. There is some dedicated libraries on Packagist if you need to lock between all your servers rather than on a single server)

Member
Tobion commented Sep 10, 2014

Which PHP methods are there for locking a file apart from flock?

Owner

None...

Member
lyrixx commented Sep 10, 2014

I forgot to push a commit :s it's done now. I added PHP Doc everywhere.

How is ensured the lock is released at script end?

I added a test for that testLockIsReleased

So if I want to actually write to the locked file, I would need to create another file handle? Because I cannot access to file handle created by the lock handler.

I guess the comment from @stof is enough.

@Tobion Worse, the class like that can't be used at all because you need to call flock yourself.

@Nicofuma I did not understand your comment. This class work out of the box. See the test for example, or look at the PR description, I put some example.

For your first question the lock is released at script end by the system because a new process is created, if you use php with FPM it should be the same I think but I'm not sure (it could depends on the OS and the strategy with the locks acquired by sub processes... but in general they should be released at the end of the sub process) and with HHVM I don't know at all.

I'm not sure to understand you. because a new process is created is wrong. PHP just release the log when the object is destructed. then you talk about FPM. FPM (or other sapi) has nothing to do here. PHP does it alone.

Member
Tobion commented Sep 10, 2014

So to sum it up: You want to introduce a Lock Handler in the Filesystem component that must not be used to lock a file? It can only be used to lock something else by means of a file. So this needs to be made clear in the phpdoc.

Again, as I said before, I think for the above usage, the class should not expect a filename as parameter, but a key for what should be locked. Then the class determines the filename based on the key automatically. As a user, I don't care what the filename is that is used for locking.

Member
Tobion commented Sep 10, 2014

E.g. new LockHandler('acme:hello')->lock(). Why would I pass a filename when I cannot actually lock the file for writing?

Member
lyrixx commented Sep 10, 2014

So to sum it up: You want to introduce a Lock Handler in the Filesystem component that must not be used to lock a file?

Initially, I wanted to add this class as LockHelper in the Command component. but you ask to change the component. So I choose the FS component because it was the closer one.

Then, the LockHandler is not for locking a file. It's for locking something (a command, an access to something, ...). But internally, we use a file and flock to do the job.

So this needs to be made clear in the phpdoc.

Sure, can you give me a sentence? thanks

Again, as I said before, I think for the above usage, the class should not expect a filename as parameter, but a key for what should be locked. Then the class determines the filename based on the key automatically. As a user, I don't care what the filename is that is used for locking.

This is quite hard to dead with. Because where to write this file by default?
As you may know cache and logs are a nightmare for some SF2 fullstack users.

Member
Tobion commented Sep 10, 2014

This is quite hard to dead with. Because where to write this file by default?

You configure a directory optionally (if not given just uses sys temp dir). But the filename is automatically determiend, e.g. hash, based on the key.

Member
lyrixx commented Sep 10, 2014

So what whould be the api:

    public function __construct($name, $folder = null)
    {
        $folder = $folder ?: sys_get_temp_dir();

        // remove "weird" characters ..
        $name = sanitize($name);

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

?

I like it. I will implement that asap.

Member
lyrixx commented Sep 10, 2014

@Tobion done.

Contributor

It's not better to use a hash instead of removing all non ascii characters?

@jderusse jderusse commented on an outdated diff Sep 10, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string $lockPath The directory to store the lock
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = preg_replace('/[^a-z0-9\._-]+/i', '-', $name);
jderusse
jderusse Sep 10, 2014 Contributor

There may be name's collision foo-bar == foo@bar == foo{#}bar
It may be saffer to suffix the name with it hash

@jderusse jderusse and 1 other commented on an outdated diff Sep 10, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string $lockPath The directory to store the lock
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = preg_replace('/[^a-z0-9\._-]+/i', '-', $name);
+
+ $lockPath = $lockPath ?: sys_get_temp_dir();
jderusse
jderusse Sep 10, 2014 Contributor

Is it possible to use a subdirectory of / tmp to avoid collision with existing files?

fabpot
fabpot Sep 10, 2014 Owner

don't forget to use sha256 and not sha1 here.

@jderusse jderusse commented on an outdated diff Sep 10, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ /**
+ * Lock the resource
+ *
+ * @param boolean $blocking
+ * @return boolean
+ */
+ public function lock($blocking = false)
+ {
+ if ($this->handle) {
+ return true;
+ }
+
+ // Set an error handler to not trigger the registered error handler if
+ // the file can not be opened.
+ set_error_handler('var_dump', 0);
+ $this->handle = @fopen($this->file, 'a+');
jderusse
jderusse Sep 10, 2014 Contributor

Why not using mode "c" ?

@jderusse jderusse and 1 other commented on an outdated diff Sep 10, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string $lockPath The directory to store the lock
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = preg_replace('/[^a-z0-9\._-]+/i', '-', $name);
+
+ $lockPath = $lockPath ?: sys_get_temp_dir();
+
+ $file = sprintf('%s/%s', $lockPath, $name);
jderusse
jderusse Sep 10, 2014 Contributor

add assertion to avoid name equals to "", "." or ".."

lyrixx
lyrixx Sep 10, 2014 Member

no need for that anymore ;)

Member
lyrixx commented Sep 10, 2014

It's not better to use a hash instead of removing all non ascii characters?

I will use a mix between ascii + hash, because I think it's always better to be able to find the generated files.

Is it possible to use a subdirectory of / tmp to avoid collision with existing files?

As there is the name + an hash, it should not conflict. See test file.

Contributor

I will use a mix between ascii + hash, because I think it's always better to be able to find the generated files.

You're right, it's nice like that.

@staabm staabm and 1 other commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string $lockPath The directory to store the lock
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = sprintf('%s-%s', preg_replace('/[^a-z0-9\._-]+/i', '-', $name), hash('sha256', $name));
staabm
staabm Sep 11, 2014 Contributor

The + within the pattern seems unnecessary

stof
stof Sep 11, 2014 Member

the goal is to avoid having lots of - following each other

Member
lyrixx commented Sep 11, 2014

I addressed all comments. I think this PR is ready for merge.

Member
lyrixx commented Sep 11, 2014

@Tobion So, what is missing to get your go?

@Tobion Tobion and 1 other commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+/**
+ * LockHandler class provides a simple abstraction to work with file lock.
+ * You can use this class to allow only one instance of a command to be running.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string $lockPath The directory to store the lock
Tobion
Tobion Sep 11, 2014 Member

string|null

lyrixx
lyrixx Sep 11, 2014 Member

fixed.

Tobion
Tobion Sep 11, 2014 Member

and missing what null means

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Filesystem;
+
+use Symfony\Component\Filesystem\Exception\IOException;
+
+/**
+ * LockHandler class provides a simple abstraction to work with file lock.
+ * You can use this class to allow only one instance of a command to be running.
Tobion
Tobion Sep 11, 2014 Member

The filesystem component has no knowledge of console commands.

I would document it like:
LockHandler class provides a simple abstraction to lock anything by means of a file lock.

A locked file is created based on the lock name when calling lock(). Other lock handlers will not be able to lock the same name until it is released (explicitly by calling release() or implicitly when the instance holding the lock is destroyed).

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ $fs = new Filesystem();
+ $fs->mkdir($lockPath);
+ }
+
+ if (!is_writable($lockPath)) {
+ throw new IOException(sprintf('The directory "%s" is not writable.', $lockPath));
+ }
+
+ $this->file = $file;
+ }
+
+ /**
+ * Lock the resource
+ *
+ * @param bool $blocking
+ * @return bool
Tobion
Tobion Sep 11, 2014 Member

missing doc what this means

Member
Tobion commented Sep 11, 2014

A test for locking on the same handler twice should be added, like

$l = new LockHandler($name);
$this->assertTrue($l->lock(true));
$this->assertTrue($l->lock(true));
@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * @return bool
+ */
+ public function lock($blocking = false)
+ {
+ if ($this->handle) {
+ return true;
+ }
+
+ // Set an error handler to not trigger the registered error handler if
+ // the file can not be opened.
+ set_error_handler('var_dump', 0);
+ $this->handle = @fopen($this->file, 'c');
+ restore_error_handler();
+
+ if (!$this->handle) {
+ throw new \RuntimeException(sprintf('Unable to fopen "%s".', $this->file));
Tobion
Tobion Sep 11, 2014 Member

why not IOException as well? also missing phpdoc

Tobion
Tobion Sep 11, 2014 Member

also use 4th argument as well

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = sprintf('%s-%s', preg_replace('/[^a-z0-9\._-]+/i', '-', $name), hash('sha256', $name));
+
+ $lockPath = $lockPath ?: sys_get_temp_dir();
+
+ $file = sprintf('%s/%s', $lockPath, $name);
+
+ if (!is_dir($lockPath)) {
+ $fs = new Filesystem();
+ $fs->mkdir($lockPath);
+ }
+
+ if (!is_writable($lockPath)) {
+ throw new IOException(sprintf('The directory "%s" is not writable.', $lockPath));
Tobion
Tobion Sep 11, 2014 Member

should set the 4th argument for getPath

Member
Tobion commented Sep 11, 2014

I'm 👍 when obove things are fixed

@Tobion Tobion commented on an outdated diff Sep 11, 2014
...ymfony/Component/Filesystem/Tests/LockHandlerTest.php
+
+ /**
+ * @expectedException Symfony\Component\Filesystem\Exception\IOException
+ * @expectedExceptionMessage The directory "/" is not writable.
+ */
+ public function testConstructWhenRepositoryIsNotWriteable()
+ {
+ new LockHandler('lock', '/');
+ }
+
+ public function testConstructSanitizeName()
+ {
+ $lock = new LockHandler('<?php echo "% hello word ! %" ?>');
+
+ $file = sprintf('%s/-php-echo-hello-word--4b3d9d0d27ddef3a78a64685dda3a963e478659a9e5240feaf7b4173a8f28d5f', sys_get_temp_dir());
+ // ensure the file does not exist before before the lock
Member
lyrixx commented Sep 11, 2014

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

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string|null $lockPath The directory to store the lock
+ * @throws IOException If the lock directory could not be created or is not writable
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = sprintf('%s-%s', preg_replace('/[^a-z0-9\._-]+/i', '-', $name), hash('sha256', $name));
+
+ $lockPath = $lockPath ?: sys_get_temp_dir();
+
+ $file = sprintf('%s/%s', $lockPath, $name);
Tobion
Tobion Sep 11, 2014 Member

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

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Filesystem;
+
+use Symfony\Component\Filesystem\Exception\IOException;
+
+/**
+ * LockHandler class provides a simple abstraction to lock anything by means of
+ * a file lock. A locked file is created based on the lock name when calling
+ * lock(). Other processes will not be able to lock the same name until it is
+ * released.
Tobion
Tobion Sep 11, 2014 Member

I updated my description

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * a file lock. A locked file is created based on the lock name when calling
+ * lock(). Other processes will not be able to lock the same name until it is
+ * released.
+ *
+ * @author Grégoire Pineau <lyrixx@lyrixx.info>
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string|null $lockPath The directory to store the lock
Tobion
Tobion Sep 11, 2014 Member

still missing what null means

@Tobion Tobion commented on an outdated diff Sep 11, 2014
src/Symfony/Component/Filesystem/LockHandler.php
+ * @author Romain Neutron <imprec@gmail.com>
+ * @author Nicolas Grekas <p@tchwork.com>
+ */
+class LockHandler
+{
+ private $file;
+ private $handle;
+
+ /**
+ * @param string $name The lock name
+ * @param string|null $lockPath The directory to store the lock
+ * @throws IOException If the lock directory could not be created or is not writable
+ */
+ public function __construct($name, $lockPath = null)
+ {
+ $name = sprintf('%s-%s', preg_replace('/[^a-z0-9\._-]+/i', '-', $name), hash('sha256', $name));
Tobion
Tobion Sep 11, 2014 Member

this will probably reach max filename length of the filesystem when the name is too long

Tobion
Tobion Sep 11, 2014 Member

so should probably be cut after a certain length of the name

@lyrixx lyrixx [Filesystem] Added a lock handler
9ad8957
Member
lyrixx commented Sep 22, 2014

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

Owner
fabpot commented Sep 22, 2014

forget about shippable, it was just a test

Member
lyrixx commented Sep 22, 2014

So it's ready for merge

Owner
fabpot commented Sep 22, 2014

👍

Contributor
rybakit commented Sep 22, 2014

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

Owner
fabpot commented Sep 22, 2014

Thank you @lyrixx.

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

1 of 2 checks passed

default Success: Travis, fabbot — Failure: app.shippable.com
Details
continuous-integration/travis-ci The Travis CI build passed
Details
@fabpot fabpot added a commit that referenced this pull request Sep 22, 2014
@fabpot fabpot feature #10475 [Filesystem] Added a LockHandler (lyrixx)
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
c85bed2
Member
lyrixx commented Sep 22, 2014

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

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.

Member
stof commented Sep 26, 2014

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

@stof oh ok, cool

@weaverryan weaverryan added a commit to symfony/symfony-docs that referenced this pull request Oct 18, 2014
@weaverryan weaverryan feature #3956 [Command] Added LockHelper (lyrixx)
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
0f34bb8
@GDmac GDmac referenced this pull request Nov 10, 2014
Closed

Regarding Lockhandler #12447

@lyrixx lyrixx deleted the lyrixx:command-lock branch Dec 8, 2015
temple commented Apr 3, 2017

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.

Member
lyrixx commented Apr 3, 2017

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

Member
lyrixx commented Apr 3, 2017

yes.

temple commented Apr 3, 2017

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment