rework locking and drop CRONJOBS_ESCALATION setting, fixes #771

Closes #771

Merge request studip/studip!592

6 files changed, 135 insertions, 159 deletions

diff --git a/app/controllers/web_migrate.php b/app/controllers/web_migrate.php
index 2637578..3553f90 100644
--- a/app/controllers/web_migrate.php
+++ b/app/controllers/web_migrate.php
@@ -26,9 +26,6 @@ class WebMigrateController extends StudipController
             true
         );
 
-        FileLock::setDirectory($GLOBALS['TMP_PATH']);
-        $this->lock = new FileLock('web-migrate');
-
         $this->setupSidebar($action);
 
         PageLayout::setTitle(_('Stud.IP Web-Migrator'));
@@ -41,38 +38,37 @@ class WebMigrateController extends StudipController
 
     public function migrate_action()
     {
-        ob_start();
-        set_time_limit(0);
-
-        $this->lock->lock(['timestamp' => time(), 'user_id' => $GLOBALS['user']->id]);
-
-        $this->migrator->migrateTo($this->target);
-
-        $this->lock->release();
-
-        $announcements = ob_get_clean();
-        PageLayout::postSuccess(
-            _('Die Datenbank wurde erfolgreich migriert.'),
-            array_filter(explode("\n", $announcements))
-        );
-
-        $_SESSION['migration-check'] = [
-            'timestamp' => time(),
-            'count'     => 0,
-        ];
-
-        $this->redirect('index');
-    }
-
-    public function release_action($target)
-    {
-        if ($this->lock->isLocked()) {
-            $this->lock->release();
-
-            PageLayout::postSuccess(_('Die Sperre wurde aufgehoben.'));
+        $lock = new FileLock('web-migrate');
+        $lock_data = ['timestamp' => time(), 'user_id' => $GLOBALS['user']->id];
+
+        if ($lock->tryLock($lock_data)) {
+            ob_start();
+            set_time_limit(0);
+
+            $this->migrator->migrateTo($this->target);
+
+            $lock->release();
+
+            $announcements = ob_get_clean();
+            PageLayout::postSuccess(
+                _('Die Datenbank wurde erfolgreich migriert.'),
+                array_filter(explode("\n", $announcements))
+            );
+
+            $_SESSION['migration-check'] = [
+                'timestamp' => time(),
+                'count'     => 0,
+            ];
+        } else {
+            $user = User::find($lock_data['user_id']);
+            PageLayout::postError(sprintf(
+                _('Die Migration wurde %s von %s bereits angestossen und läuft noch.'),
+                reltime($lock_data['timestamp']),
+                htmlReady($user ? $user->getFullName() : _('unbekannt'))
+            ));
         }
 
-        $this->redirect($this->url_for('index', compact('target')));
+        $this->redirect('index');
     }
 
     public function history_action()
diff --git a/app/views/web_migrate/index.php b/app/views/web_migrate/index.php
index 6bba52f..5de2679 100644
--- a/app/views/web_migrate/index.php
+++ b/app/views/web_migrate/index.php
@@ -54,25 +54,7 @@
         <tfoot>
             <tr>
                 <td colspan="4">
-                <? if ($lock->isLocked($lock_data)):
-                    $user = User::find($lock_data['user_id']);
-                ?>
-                    <?= MessageBox::info(sprintf(
-                        _('Die Migration wurde %s von %s bereits angestossen und läuft noch.'),
-                        reltime($lock_data['timestamp']),
-                        htmlReady($user ? $user->getFullName() : _('unbekannt'))
-                    ), [
-                        sprintf(
-                            _('Sollte während der Migration ein Fehler aufgetreten sein, so können Sie '
-                            . 'diese Sperre durch den unten stehenden Link oder das Löschen der Datei '
-                            . '<em>%s</em> auflösen.'),
-                            $lock->getFilename()
-                        )
-                    ]) ?>
-                    <?= Studip\LinkButton::create(_('Sperre aufheben'), $controller->url_for('release', $target)) ?>
-                <? else: ?>
                     <?= Studip\Button::createAccept(_('Starten'), 'start')?>
-                <? endif; ?>
                 </td>
             </tr>
         </tfoot>
diff --git a/db/migrations/5.2.5_drop_cronjobs_escalation.php b/db/migrations/5.2.5_drop_cronjobs_escalation.php
new file mode 100644
index 0000000..848eddd
--- /dev/null
+++ b/db/migrations/5.2.5_drop_cronjobs_escalation.php
@@ -0,0 +1,31 @@
+<?php
+
+class DropCronjobsEscalation extends Migration
+{
+    public function description()
+    {
+        return 'Drop CRONJOBS_ESCALATION system setting';
+    }
+
+    public function up()
+    {
+        $query = "DELETE `config`, `config_values`
+                  FROM `config` LEFT JOIN `config_values` USING (`field`)
+                  WHERE `field` = 'CRONJOBS_ESCALATION'";
+        DBManager::get()->exec($query);
+    }
+
+    public function down()
+    {
+        $query = 'INSERT INTO `config` (`field`, `value`, `type`, `section`, `mkdate`, `chdate`, `description`)
+                  VALUES (:name, :value, :type, :section, UNIX_TIMESTAMP(), UNIX_TIMESTAMP(), :description)';
+        $statement = DBManager::get()->prepare($query);
+        $statement->execute([
+            ':name'        => 'CRONJOBS_ESCALATION',
+            ':description' => 'Gibt an, nach wievielen Sekunden ein Cronjob als steckengeblieben angesehen wird',
+            ':section'     => 'global',
+            ':type'        => 'integer',
+            ':value'       => '300'
+        ]);
+    }
+}
diff --git a/lib/classes/CronjobScheduler.class.php b/lib/classes/CronjobScheduler.class.php
index 86f14b5..67db94c 100644
--- a/lib/classes/CronjobScheduler.class.php
+++ b/lib/classes/CronjobScheduler.class.php
@@ -46,15 +46,11 @@ class CronjobScheduler
         return self::$instance;
     }
 
-    protected $lock;
-
     /**
      * Private constructor to ensure the singleton pattern is used correctly.
      */
     private function __construct()
     {
-        FileLock::setDirectory($GLOBALS['TMP_PATH']);
-        $this->lock = new FileLock('studip-cronjob');
     }
 
     /**
@@ -254,54 +250,16 @@ class CronjobScheduler
             return;
         }
 
-        $escalation_time = Config::get()->CRONJOBS_ESCALATION;
+        $lock = new FileLock('studip-cronjob');
 
         // Check whether a previous cronjob worker is still running.
-        if ($this->lock->isLocked($data)) {
-            // Running but not yet escalated -> let it run
-            if ($data['timestamp'] + $escalation_time > time()) {
-                return;
-            }
-
-            // Load locked schedule
-            $schedule = CronjobSchedule::find($data['schedule_id']);
-
-            // If we discovered a deadlock release it
-            if ($schedule) {
-                // Deactivate schedule
-                $schedule->deactivate();
-
-                // Adjust log
-                $log = CronjobLog::find($data['log_id']);
-                if ($log) {
-                    $log->duration  = time() - $data['timestamp'];
-                    $log->exception = new Exception('Cronjob has escalated');
-                    $log->store();
-                }
-
-                // Inform roots about the escalated cronjob
-                $subject = sprintf('[Cronjobs] %s: %s',
-                                   _('Eskalierte Ausführung'),
-                                   $schedule->title);
-
-                $message = sprintf(_('Der Cronjob "%s" wurde deaktiviert, da '
-                                    .'seine Ausführungsdauer die maximale '
-                                    .'Ausführungszeit von %u Sekunden '
-                                    .'überschritten hat.') . "\n",
-                                   $schedule->title,
-                                   $escalation_time);
-
-                $this->sendMailToRoots($subject, $message);
-            }
-
-            // Release lock
-            $this->lock->release();
+        if (!$lock->tryLock()) {
+            return;
         }
 
         // Find all schedules that are due to execute and which task is active
         $temp = CronjobSchedule::findBySQL('active = 1 AND next_execution <= UNIX_TIMESTAMP() '
                                           .'ORDER BY priority DESC, next_execution ASC');
-#        $temp = SimpleORMapCollection::createFromArray($temp);
         $schedules = array_filter($temp, function ($schedule) { return $schedule->task->active; });
 
         if (count($schedules) === 0) {
@@ -323,15 +281,6 @@ class CronjobScheduler
                 $log->duration    = -1;
                 $log->store();
 
-                set_time_limit($escalation_time);
-
-                // Activate the file lock and store the current timestamp,
-                // schedule id and according log id in it
-                $this->lock->lock([
-                    'schedule_id' => $schedule->schedule_id,
-                    'log_id'      => $log->log_id,
-                ]);
-
                 // Start capturing output and measuring duration
                 ob_start();
                 $start_time = microtime(true);
@@ -372,7 +321,7 @@ class CronjobScheduler
         }
 
         // Release lock
-        $this->lock->release();
+        $lock->release();
     }
 
     /**
diff --git a/lib/classes/FileLock.class.php b/lib/classes/FileLock.class.php
index cf6c00a..064d41e 100644
--- a/lib/classes/FileLock.class.php
+++ b/lib/classes/FileLock.class.php
@@ -20,24 +20,7 @@
 
 class FileLock
 {
-    protected static $directory = '';
-
-    /**
-     * Sets a new base path for the locks.
-     *
-     * @param String $directory
-     * @throws RuntimeException if provided directory is either not existant
-     *         or is not a directory or is not writable.
-     */
-    public static function setDirectory($directory)
-    {
-        if (!file_exists($directory) || !is_dir($directory) || !is_writable($directory)) {
-            throw new RuntimeException('Passed directory is not an actual directory or is not writable.');
-        }
-        self::$directory = rtrim($directory, '/') . '/';
-    }
-
-    protected $filename;
+    protected $file;
 
     /**
      * Constructs a new lock object with the provided id.
@@ -46,55 +29,46 @@ class FileLock
      */
     public function __construct($id)
     {
-        $this->filename = self::$directory . '.' . $id . '.json';
-    }
-    
-    /**
-     * Returns the filename of the lock.
-     *
-     * @return String Filename of the lock
-     */
-    public function getFilename()
-    {
-        return $this->filename;
+        $this->file = fopen("{$GLOBALS['TMP_PATH']}/$id.json", 'c+');
+
+        if (!$this->file) {
+            throw new RuntimeException('failed to create lock file.');
+        }
     }
     
     /**
-     * Establish or renew the current lock. Provided lock information will
-     * be stored with the lock.
+     * Try to aquire a file lock. The provided lock information will
+     * be stored with the lock. If the lock cannot be aquired, the
+     * lock information in $data is updated from the lock file.
      *
-     * @param Array $data Additional information to bestore with the lock
+     * @param array $data additional data to be stored with the lock
+     * @return boolean true on success or false on failure
      */
-    public function lock($data = [])
+    public function tryLock(&$data = [])
     {
-        $data['timestamp'] = time();
-        file_put_contents($this->filename, json_encode($data));
-    }
+        rewind($this->file);
+
+        if (flock($this->file, LOCK_EX | LOCK_NB)) {
+            ftruncate($this->file, 0);
+            fwrite($this->file, json_encode($data));
+            fflush($this->file);
+
+            return true;
+        } else {
+            $json = stream_get_contents($this->file);
+            $data = json_decode($json, true);
 
-    /**
-     * Tests whether the lock is in use. Returns lock information in
-     * $lock_data.
-     *
-     * @param mixed $lock_data Information stored in lock
-     * @return bool Indicates whether the lock is active or not
-     */
-    public function isLocked(&$lock_data = null)
-    {
-        if (!file_exists($this->filename)) {
             return false;
         }
-
-        $lock_data = json_decode(file_get_contents($this->filename), true);
-        return true;
     }
 
     /**
      * Releases a previously obtained lock
+     *
+     * @return boolean true on success or false on failure
      */
     public function release()
     {
-        if (file_exists($this->filename)) {
-            unlink($this->filename);
-        }
+        return flock($this->file, LOCK_UN);
     }
-}
\ No newline at end of file
+}
diff --git a/tests/unit/lib/classes/FileLockTest.php b/tests/unit/lib/classes/FileLockTest.php
new file mode 100644
index 0000000..b593794
--- /dev/null
+++ b/tests/unit/lib/classes/FileLockTest.php
@@ -0,0 +1,44 @@
+<?php
+/*
+ * Copyright (c) 2022  Elmar Ludwig
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
+ */
+
+class FileLockTest extends \Codeception\Test\Unit
+{
+    public function testAquireLock()
+    {
+        $lock = new FileLock('test');
+        $lock2 = new FileLock('test');
+
+        $this->assertTrue($lock->tryLock());
+        $lock->release();
+
+        $this->assertTrue($lock2->tryLock());
+        $lock2->release();
+    }
+
+    public function testBusyLock()
+    {
+        $lock = new FileLock('test');
+        $lock2 = new FileLock('test');
+
+        $data = ['foo' => '42'];
+        $this->assertTrue($lock->tryLock($data));
+
+        // test updating a lock
+        $data = ['foo' => 'bar'];
+        $this->assertTrue($lock->tryLock($data));
+
+        // aquiring the lock should fail
+        $data = [];
+        $this->assertFalse($lock2->tryLock($data));
+        $this->assertEquals('bar', $data['foo']);
+
+        $lock->release();
+    }
+}