From c054faf90288a75fc3680480434ba93b7f5b287b Mon Sep 17 00:00:00 2001
From: Elmar Ludwig <elmar.ludwig@uni-osnabrueck.de>
Date: Tue, 10 May 2022 22:36:06 +0000
Subject: [PATCH] rework locking and drop CRONJOBS_ESCALATION setting, fixes
#771
Closes #771
Merge request studip/studip!592
---
app/controllers/web_migrate.php | 62 +++++++-------
app/views/web_migrate/index.php | 18 -----
.../5.2.5_drop_cronjobs_escalation.php | 31 +++++++
lib/classes/CronjobScheduler.class.php | 59 +-------------
lib/classes/FileLock.class.php | 80 +++++++------------
tests/unit/lib/classes/FileLockTest.php | 44 ++++++++++
6 files changed, 135 insertions(+), 159 deletions(-)
create mode 100644 db/migrations/5.2.5_drop_cronjobs_escalation.php
create mode 100644 tests/unit/lib/classes/FileLockTest.php
diff --git a/app/controllers/web_migrate.php b/app/controllers/web_migrate.php
index 2637578d98d..3553f9041a4 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 6bba52fbf24..5de26791c10 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 00000000000..848eddd4bf3
--- /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 86f14b5d5ff..67db94c6d4d 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 cf6c00ae6e7..064d41ef9cf 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 00000000000..b593794e32d
--- /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();
+ }
+}
--
GitLab