Fix lock storage bug (#171)

docker-compose.yml

@@ -3,8 +3,7 @@ version: '3'
 services:
     php56:
         build:
-            context: .
-            dockerfile: ./docker/php56/Dockerfile
+            context: ./docker/php56
         working_dir: /var/www/html
         command: >
             sh -c "
@@ -17,3 +16,19 @@ services:
         volumes:
             - .:/var/www/html
             - ./docker/php56/php.ini:/usr/local/etc/php/php.ini:ro
+
+    dev_tools:
+        build:
+            context: ./docker/dev-tools
+        working_dir: /var/www/html
+        command: >
+            sh -c "
+                chown -R www-data:www-data /var/www/.composer \
+                && su-exec www-data composer bin all install -o \
+                && echo 'Logs from /var/log/php/error.log:' \
+                && touch /var/log/php/error.log \
+                && tail -f /var/log/php/error.log
+            "
+        volumes:
+            - .:/var/www/html
+            - ./docker/php56/php.ini:/usr/local/etc/php/php.ini:ro

docker/dev-tools/Dockerfile

@@ -0,0 +1,18 @@
+FROM php:7.2-cli-alpine
+
+RUN apk add --no-cache \
+        su-exec && \
+    docker-php-ext-install -j$(nproc) \
+        opcache
+
+RUN mkdir -p \
+        /var/log/php \
+        /var/www/.composer \
+    && touch /var/log/php/error.log \
+    && chown www-data:www-data \
+        /var/log/php/error.log \
+        /var/www/.composer
+
+COPY --from=composer:1.8 /usr/bin/composer /usr/bin/composer
+ENV COMPOSER_HOME /var/www/.composer
+RUN su-exec www-data composer global require hirak/prestissimo -a && rm -rf /var/www/.composer/cache

docker/php56/Dockerfile

@@ -3,7 +3,8 @@ FROM php:5.6-cli-alpine
 RUN apk add --no-cache \
         su-exec && \
     docker-php-ext-install -j$(nproc) \
-        opcache
+        opcache \
+        sysvsem
 
 RUN mkdir -p \
         /var/log/php \

src/Event.php

@@ -12,8 +12,10 @@
 use Crunz\Pinger\PingableInterface;
 use Crunz\Pinger\PingableTrait;
 use SuperClosure\Serializer;
+use Symfony\Component\Lock\Exception\InvalidArgumentException;
 use Symfony\Component\Lock\Factory;
 use Symfony\Component\Lock\Lock;
+use Symfony\Component\Lock\Store\FlockStore;
 use Symfony\Component\Lock\StoreInterface;
 use Symfony\Component\Process\Process;
 
@@ -27,13 +29,6 @@ class Event implements PingableInterface
 {
     use PingableTrait;
 
-    /**
-     * Indicates if the command should not overlap itself.
-     *
-     * @var bool
-     */
-    public $preventOverlapping = false;
-
     /**
      * The location that output should be sent to.
      *
@@ -158,6 +153,13 @@ class Event implements PingableInterface
         'month' => 4,
         'week' => 5,
     ];
+
+    /**
+     * Indicates if the command should not overlap itself.
+     *
+     * @var bool
+     */
+    private $preventOverlapping = false;
     /** @var ClockInterface */
     private static $clock;
 
@@ -168,15 +170,10 @@ class Event implements PingableInterface
      * @var Factory|null
      */
     private $lockFactory;
-
-    /**
-     * Contains the timestamp of the last lock refresh.
-     *
-     * @var int
-     */
-    private $lastLockRefresh = 0;
-
+    /** @var string[] */
     private $wholeOutput = [];
+    /** @var Lock */
+    private $lock;
 
     /**
      * Create a new event instance.
@@ -754,14 +751,16 @@ public function user($user)
      */
     public function preventOverlapping(StoreInterface $store = null)
     {
+        $lockStore = $store ?: $this->createDefaultLockStore();
         $this->preventOverlapping = true;
-        if (null !== $store) {
-            $this->lockFactory = new Factory($store);
-        }
+        $this->lockFactory = new Factory($lockStore);
 
         // Skip the event if it's locked (processing)
         $this->skip(function () {
-            return $this->isLocked();
+            $lock = $this->createLockObject();
+            $lock->acquire();
+
+            return !$lock->isAcquired();
         });
 
         // Delete the lock file when the event is completed
@@ -1047,57 +1046,6 @@ public function afterCallbacks()
         return $this->afterCallbacks;
     }
 
-    /**
-     * Check if another instance of the event is still running.
-     *
-     * @return bool
-     */
-    public function isLocked()
-    {
-        if (null !== $this->lockFactory) {
-            $lock = $this->createLockObject();
-
-            return $lock->isAcquired();
-        }
-
-        // If no symfony lock object is given, fall back to file system locks.
-        $pid = $this->lastPid();
-        $hasPid = (null !== $pid);
-
-        // No POSIX on Windows
-        if ($this->isWindows()) {
-            return $hasPid;
-        }
-
-        return ($hasPid && posix_getsid($pid)) ? true : false;
-    }
-
-    /**
-     * Get the last process Id of the event.
-     *
-     * @return int
-     */
-    public function lastPid()
-    {
-        $lock_file = $this->lockFile();
-
-        return file_exists($lock_file) ? (int) trim(file_get_contents($lock_file)) : null;
-    }
-
-    /**
-     * Get the lock file path for the task.
-     *
-     * @return string
-     */
-    public function lockFile()
-    {
-        if (null !== $this->lockFactory) {
-            throw new \BadMethodCallException('We are not using file based locking, but instead symfony/lock');
-        }
-
-        return rtrim(sys_get_temp_dir(), '/') . '/crunz-' . md5($this->buildCommand());
-    }
-
     /**
      * If this event is prevented from overlapping, this method should be called regularly to refresh the lock.
      */
@@ -1107,14 +1055,18 @@ public function refreshLock()
             return;
         }
 
-        if (null === $this->lockFactory) {
-            // In case of file based locking there is nothing to do.
+        $lock = $this->createLockObject();
+        $remainingLifetime = $lock->getRemainingLifetime();
+
+        // Lock will never expire
+        if (null === $remainingLifetime) {
+            return;
         }
 
-        // Refresh lock every 10s
-        $lockRefreshNeeded = $this->lastLockRefresh < (time() - 10);
+        // Refresh 15s before lock expiration
+        $lockRefreshNeeded = $remainingLifetime < 15;
         if ($lockRefreshNeeded) {
-            $this->createLockObject()->refresh();
+            $lock->refresh();
         }
     }
 
@@ -1125,33 +1077,27 @@ public function refreshLock()
      */
     protected function createLockObject()
     {
-        if (null === $this->lockFactory) {
-            throw new \BadMethodCallException(
-                'No lock factory. Please call preventOverlapping() with lock storage first.'
-            );
-        }
+        $this->checkLockFactory();
+
+        if (null === $this->lock) {
+            $ttl = 30;
 
-        $ttl = 30;
+            $this->lock = $this->lockFactory
+                ->createLock($this->lockKey(), $ttl);
+        }
 
-        return $this->lockFactory->createLock('crunz-' . md5($this->buildCommand()), $ttl);
+        return $this->lock;
     }
 
     /**
      * Release the lock after the command completed.
      */
     protected function releaseLock()
     {
-        if (null !== $this->lockFactory) {
-            $lock = $this->createLockObject();
-            $lock->release();
+        $this->checkLockFactory();
 
-            return;
-        }
-
-        $lockfile = $this->lockFile();
-        if (file_exists($lockfile)) {
-            unlink($lockfile);
-        }
+        $lock = $this->createLockObject();
+        $lock->release();
     }
 
     /**
@@ -1271,14 +1217,53 @@ protected function applyMask($unit)
 
     /**
      * Lock the event.
+     */
+    protected function lock()
+    {
+        $lock = $this->createLockObject();
+        $lock->acquire();
+    }
+
+    /**
+     * @return FlockStore
      *
-     * @param \Crunz\Event $event
-     *
+     * @throws Exception\CrunzException
+     */
+    private function createDefaultLockStore()
+    {
+        try {
+            $lockPath = Path::create(
+                [
+                    \sys_get_temp_dir(),
+                    '.crunz',
+                ]
+            );
+
+            $store = new FlockStore($lockPath->toString());
+        } catch (InvalidArgumentException $exception) {
+            // Fallback to system temp dir
+            $lockPath = Path::create([\sys_get_temp_dir()]);
+            $store = new FlockStore($lockPath->toString());
+        }
+
+        return $store;
+    }
+
+    /**
      * @return string
      */
-    protected function lock()
+    private function lockKey()
     {
-        file_put_contents($this->lockFile(), $this->process->getPid());
+        return 'crunz-' . \md5($this->buildCommand());
+    }
+
+    private function checkLockFactory()
+    {
+        if (null === $this->lockFactory) {
+            throw new \BadMethodCallException(
+                'No lock factory. Please call preventOverlapping() first.'
+            );
+        }
     }
 
     /** @return ClockInterface */

src/EventRunner.php

@@ -147,10 +147,14 @@ protected function manageStartedEvents()
         while ($this->schedules) {
             foreach ($this->schedules as $scheduleKey => $schedule) {
                 $events = $schedule->events();
+                // 10% chance that refresh will be called
+                $refreshLocks = (\rand(1, 100) <= 10);
 
                 /** @var Event $event */
                 foreach ($events as $eventKey => $event) {
-                    $event->refreshLock();
+                    if ($refreshLocks) {
+                        $event->refreshLock();
+                    }
 
                     $proc = $event->getProcess();
                     if ($proc->isRunning()) {

tests/Unit/EventTest.php

@@ -7,6 +7,8 @@
 use Crunz\Tests\TestCase\TestClock;
 use PHPUnit\Framework\TestCase;
 use SuperClosure\Serializer;
+use Symfony\Component\Lock\Store\SemaphoreStore;
+use Symfony\Component\Lock\StoreInterface;
 
 class EventTest extends TestCase
 {
@@ -371,6 +373,43 @@ public function wholeOutputCatchesStdoutAndStderr()
         );
     }
 
+    /** @test */
+    public function taskWillPreventOverlappingWithDefaultStore()
+    {
+        $this->assertPreventOverlapping();
+    }
+
+    /** @test */
+    public function taskWillPreventOverlappingWithSemaphoreStore()
+    {
+        if (!\extension_loaded('sysvsem')) {
+            $this->markTestSkipped('Semaphore extension not installed.');
+        }
+
+        $this->assertPreventOverlapping(new SemaphoreStore());
+    }
+
+    private function assertPreventOverlapping(StoreInterface $store = null)
+    {
+        $event = $this->createPreventOverlappingEvent($store);
+        $event2 = $this->createPreventOverlappingEvent($store);
+
+        $event->start();
+
+        $this->assertFalse($event2->isDue(new \DateTimeZone('UTC')));
+    }
+
+    private function createPreventOverlappingEvent(StoreInterface $store = null)
+    {
+        $command = "php -r 'sleep(2);'";
+
+        $event = new Event(\uniqid('c', true), $command);
+        $event->preventOverlapping($store);
+        $event->everyMinute();
+
+        return $event;
+    }
+
     private function setClockNow(\DateTimeImmutable $dateTime)
     {
         $testClock = new TestClock($dateTime);