Merge pull request #24696 from owncloud/lock-jobs-while-executing

Lock jobs while executing them, to allow multiple executors to run in…

config/config.sample.php

@@ -614,17 +614,6 @@
  */
 'cron_log' => true,
 
-/**
- * Location of the lock file for cron executions can be specified here.
- * Default is within the tmp directory. The file is named in the following way:
- * owncloud-server-$INSTANCEID-cron.lock
- * where $INSTANCEID is the string specified in the ``instanceid`` field.
- * Because the cron lock file is accessed at regular intervals, it may prevent
- * enabled disk drives from spinning down. A different location for this file
- * can solve such issues.
- */
-'cron.lockfile.location' => '',
-
 /**
  * Enables log rotation and limits the total size of logfiles. The default is 0,
  * or no rotation. Specify a size in bytes, for example 104857600 (100 megabytes

cron.php

@@ -100,34 +100,11 @@
 			}
 		}
 
-		$instanceId = $config->getSystemValue('instanceid');
-		$lockFileName = 'owncloud-server-' . $instanceId . '-cron.lock';
-		$lockDirectory = $config->getSystemValue('cron.lockfile.location', sys_get_temp_dir());
-		$lockDirectory = rtrim($lockDirectory, '\\/');
-		$lockFile = $lockDirectory . '/' . $lockFileName;
-
-		if (!file_exists($lockFile)) {
-			touch($lockFile);
-		}
-
 		// We call ownCloud from the CLI (aka cron)
 		if ($appMode != 'cron') {
 			\OCP\BackgroundJob::setExecutionType('cron');
 		}
 
-		// open the file and try to lock it. If it is not locked, the background
-		// job can be executed, otherwise another instance is already running
-		$fp = fopen($lockFile, 'w');
-		$isLocked = flock($fp, LOCK_EX|LOCK_NB, $wouldBlock);
-
-		// check if backgroundjobs is still running. The wouldBlock check is
-		// needed on systems with advisory locking, see
-		// http://php.net/manual/en/function.flock.php#45464
-		if (!$isLocked || $wouldBlock) {
-			echo "Another instance of cron.php is still running!" . PHP_EOL;
-			exit(1);
-		}
-
 		// Work
 		$jobList = \OC::$server->getJobList();
 
@@ -138,6 +115,7 @@
 		$executedJobs = [];
 		while ($job = $jobList->getNext()) {
 			if (isset($executedJobs[$job->getId()])) {
+				$jobList->unlockJob($job);
 				break;
 			}
 
@@ -154,10 +132,6 @@
 			}
 		}
 
-		// unlock the file
-		flock($fp, LOCK_UN);
-		fclose($fp);
-
 	} else {
 		// We call cron.php from some website
 		if ($appMode == 'cron') {

db_structure.xml

@@ -968,12 +968,29 @@
 			</field>
 
 			<field>
+				<!-- timestamp when the job was executed the last time -->
 				<name>last_run</name>
 				<type>integer</type>
 				<default></default>
 				<notnull>false</notnull>
 			</field>
 
+			<field>
+				<!-- timestamp when the job was checked if it needs execution the last time -->
+				<name>last_checked</name>
+				<type>integer</type>
+				<default></default>
+				<notnull>false</notnull>
+			</field>
+
+			<field>
+				<!-- timestamp when the job was reserved the last time, 1 day timeout -->
+				<name>reserved_at</name>
+				<type>integer</type>
+				<default></default>
+				<notnull>false</notnull>
+			</field>
+
 			<index>
 				<name>job_class_index</name>
 				<field>

lib/private/AppFramework/Db/Db.php

@@ -29,6 +29,7 @@
 use OCP\DB\QueryBuilder\IQueryBuilder;
 use OCP\IDb;
 use OCP\IDBConnection;
+use OCP\PreConditionNotMetException;
 
 /**
  * @deprecated use IDBConnection directly, will be removed in ownCloud 10
@@ -157,12 +158,26 @@ public function insertIfNotExist($table, $input, array $compare = null) {
 	 * @param array $updatePreconditionValues ensure values match preconditions (column name => value)
 	 * @return int number of new rows
 	 * @throws \Doctrine\DBAL\DBALException
-	 * @throws PreconditionNotMetException
+	 * @throws PreConditionNotMetException
 	 */
 	public function setValues($table, array $keys, array $values, array $updatePreconditionValues = []) {
 		return $this->connection->setValues($table, $keys, $values, $updatePreconditionValues);
 	}
 
+	/**
+	 * @inheritdoc
+	 */
+	public function lockTable($tableName) {
+		$this->connection->lockTable($tableName);
+	}
+
+	/**
+	 * @inheritdoc
+	 */
+	public function unlockTable() {
+		$this->connection->unlockTable();
+	}
+
 	/**
 	 * Start a transaction
 	 */

lib/private/BackgroundJob/JobList.php

@@ -25,27 +25,34 @@
 namespace OC\BackgroundJob;
 
 use OCP\AppFramework\QueryException;
+use OCP\AppFramework\Utility\ITimeFactory;
 use OCP\BackgroundJob\IJob;
 use OCP\BackgroundJob\IJobList;
 use OCP\AutoloadNotAllowedException;
 use OCP\DB\QueryBuilder\IQueryBuilder;
+use OCP\IConfig;
+use OCP\IDBConnection;
 
 class JobList implements IJobList {
-	/** @var \OCP\IDBConnection */
+
+	/** @var IDBConnection */
 	protected $connection;
 
-	/**
-	 * @var \OCP\IConfig $config
-	 */
+	/**@var IConfig */
 	protected $config;
 
+	/**@var ITimeFactory */
+	protected $timeFactory;
+
 	/**
-	 * @param \OCP\IDBConnection $connection
-	 * @param \OCP\IConfig $config
+	 * @param IDBConnection $connection
+	 * @param IConfig $config
+	 * @param ITimeFactory $timeFactory
 	 */
-	public function __construct($connection, $config) {
+	public function __construct(IDBConnection $connection, IConfig $config, ITimeFactory $timeFactory) {
 		$this->connection = $connection;
 		$this->config = $config;
+		$this->timeFactory = $timeFactory;
 	}
 
 	/**
@@ -71,6 +78,7 @@ public function add($job, $argument = null) {
 					'class' => $query->createNamedParameter($class),
 					'argument' => $query->createNamedParameter($argument),
 					'last_run' => $query->createNamedParameter(0, IQueryBuilder::PARAM_INT),
+					'last_checked' => $query->createNamedParameter($this->timeFactory->getTime(), IQueryBuilder::PARAM_INT),
 				]);
 			$query->execute();
 		}
@@ -167,45 +175,40 @@ public function getAll() {
 	 * @return IJob|null
 	 */
 	public function getNext() {
-		$lastId = $this->getLastJob();
-
 		$query = $this->connection->getQueryBuilder();
 		$query->select('*')
 			->from('jobs')
-			->where($query->expr()->lt('id', $query->createNamedParameter($lastId, IQueryBuilder::PARAM_INT)))
-			->orderBy('id', 'DESC')
+			->where($query->expr()->lte('reserved_at', $query->createNamedParameter($this->timeFactory->getTime() - 12 * 3600, IQueryBuilder::PARAM_INT)))
+			->orderBy('last_checked', 'ASC')
 			->setMaxResults(1);
+
+		$update = $this->connection->getQueryBuilder();
+		$update->update('jobs')
+			->set('reserved_at', $update->createNamedParameter($this->timeFactory->getTime()))
+			->set('last_checked', $update->createNamedParameter($this->timeFactory->getTime()))
+			->where($update->expr()->eq('id', $update->createParameter('jobid')));
+
+		$this->connection->lockTable('jobs');
 		$result = $query->execute();
 		$row = $result->fetch();
 		$result->closeCursor();
 
 		if ($row) {
-			$jobId = $row['id'];
+			$update->setParameter('jobid', $row['id']);
+			$update->execute();
+			$this->connection->unlockTable();
+
 			$job = $this->buildJob($row);
-		} else {
-			//begin at the start of the queue
-			$query = $this->connection->getQueryBuilder();
-			$query->select('*')
-				->from('jobs')
-				->orderBy('id', 'DESC')
-				->setMaxResults(1);
-			$result = $query->execute();
-			$row = $result->fetch();
-			$result->closeCursor();
-
-			if ($row) {
-				$jobId = $row['id'];
-				$job = $this->buildJob($row);
-			} else {
-				return null; //empty job list
+
+			if ($job === null) {
+				// Background job from disabled app, try again.
+				return $this->getNext();
 			}
-		}
 
-		if (is_null($job)) {
-			$this->removeById($jobId);
-			return $this->getNext();
-		} else {
 			return $job;
+		} else {
+			$this->connection->unlockTable();
+			return null;
 		}
 	}
 
@@ -267,13 +270,30 @@ private function buildJob($row) {
 	 * @param IJob $job
 	 */
 	public function setLastJob($job) {
+		$this->unlockJob($job);
 		$this->config->setAppValue('backgroundjob', 'lastjob', $job->getId());
 	}
 
+	/**
+	 * Remove the reservation for a job
+	 *
+	 * @param IJob $job
+	 */
+	public function unlockJob($job) {
+		$query = $this->connection->getQueryBuilder();
+		$query->update('jobs')
+			->set('reserved_at', $query->expr()->literal(0, IQueryBuilder::PARAM_INT))
+			->where($query->expr()->eq('id', $query->createNamedParameter($job->getId(), IQueryBuilder::PARAM_INT)));
+		$query->execute();
+	}
+
 	/**
 	 * get the id of the last ran job
 	 *
 	 * @return int
+	 * @deprecated 9.1.0 - The functionality behind the value is deprecated, it
+	 *    only tells you which job finished last, but since we now allow multiple
+	 *    executors to run in parallel, it's not used to calculate the next job.
 	 */
 	public function getLastJob() {
 		return (int) $this->config->getAppValue('backgroundjob', 'lastjob', 0);

lib/private/DB/Adapter.php

@@ -57,6 +57,26 @@ public function fixupStatement($statement) {
 		return $statement;
 	}
 
+	/**
+	 * Create an exclusive read+write lock on a table
+	 *
+	 * @param string $tableName
+	 * @since 9.1.0
+	 */
+	public function lockTable($tableName) {
+		$this->conn->beginTransaction();
+		$this->conn->executeUpdate('LOCK TABLE `' .$tableName . '` IN EXCLUSIVE MODE');
+	}
+
+	/**
+	 * Release a previous acquired lock again
+	 *
+	 * @since 9.1.0
+	 */
+	public function unlockTable() {
+		$this->conn->commit();
+	}
+
 	/**
 	 * Insert a row if the matching row does not exists.
 	 *

lib/private/DB/AdapterMySQL.php

@@ -24,6 +24,18 @@
 namespace OC\DB;
 
 class AdapterMySQL extends Adapter {
+
+	/**
+	 * @param string $tableName
+	 */
+	public function lockTable($tableName) {
+		$this->conn->executeUpdate('LOCK TABLES `' .$tableName . '` WRITE');
+	}
+
+	public function unlockTable() {
+		$this->conn->executeUpdate('UNLOCK TABLES');
+	}
+
 	public function fixupStatement($statement) {
 		$statement = str_replace(' ILIKE ', ' COLLATE utf8_general_ci LIKE ', $statement);
 		return $statement;

lib/private/DB/AdapterSqlite.php

@@ -27,6 +27,18 @@
 namespace OC\DB;
 
 class AdapterSqlite extends Adapter {
+
+	/**
+	 * @param string $tableName
+	 */
+	public function lockTable($tableName) {
+		$this->conn->executeUpdate('BEGIN EXCLUSIVE TRANSACTION');
+	}
+
+	public function unlockTable() {
+		$this->conn->executeUpdate('COMMIT TRANSACTION');
+	}
+
 	public function fixupStatement($statement) {
 		$statement = preg_replace('( I?LIKE \?)', '$0 ESCAPE \'\\\'', $statement);
 		$statement = preg_replace('/`(\w+)` ILIKE \?/', 'LOWER($1) LIKE LOWER(?)', $statement);