Merge pull request #6150 from owncloud/backgroundjob-log-exception

Remove background jobs that are giving errors

cron.php

@@ -50,6 +50,8 @@ try {
 
 	session_write_close();
 
+	$logger = \OC_Log::$object;
+
 	// Don't do anything if ownCloud has not been installed
 	if (!OC_Config::getValue('installed', false)) {
 		exit(0);
@@ -98,7 +100,7 @@ try {
 		$jobList = new \OC\BackgroundJob\JobList();
 		$jobs = $jobList->getAll();
 		foreach ($jobs as $job) {
-			$job->execute($jobList);
+			$job->execute($jobList, $logger);
 		}
 	} else {
 		// We call cron.php from some website
@@ -109,7 +111,7 @@ try {
 			// Work and success :-)
 			$jobList = new \OC\BackgroundJob\JobList();
 			$job = $jobList->getNext();
-			$job->execute($jobList);
+			$job->execute($jobList, $logger);
 			$jobList->setLastJob($job);
 			OC_JSON::success();
 		}

lib/private/backgroundjob/job.php

@@ -9,16 +9,35 @@
 namespace OC\BackgroundJob;
 
 abstract class Job {
+	/**
+	 * @var int $id
+	 */
 	protected $id;
+
+	/**
+	 * @var int $lastRun
+	 */
 	protected $lastRun;
+
+	/**
+	 * @var mixed $argument
+	 */
 	protected $argument;
 
 	/**
 	 * @param JobList $jobList
+	 * @param \OC\Log $logger
 	 */
-	public function execute($jobList) {
+	public function execute($jobList, $logger = null) {
 		$jobList->setLastRun($this);
-		$this->run($this->argument);
+		try {
+			$this->run($this->argument);
+		} catch (\Exception $e) {
+			if ($logger) {
+				$logger->error('Error while running background job: ' . $e->getMessage());
+			}
+			$jobList->remove($this, $this->argument);
+		}
 	}
 
 	abstract protected function run($argument);

lib/private/backgroundjob/queuedjob.php

@@ -20,9 +20,10 @@ abstract class QueuedJob extends Job {
 	 * run the job, then remove it from the joblist
 	 *
 	 * @param JobList $jobList
+	 * @param \OC\Log $logger
 	 */
-	public function execute($jobList) {
+	public function execute($jobList, $logger = null) {
 		$jobList->remove($this);
-		$this->run($this->argument);
+		parent::execute($jobList, $logger);
 	}
 }

lib/private/backgroundjob/timedjob.php

@@ -31,11 +31,11 @@ abstract class TimedJob extends Job {
 	 * run the job if
 	 *
 	 * @param JobList $jobList
+	 * @param \OC\Log $logger
 	 */
-	public function execute($jobList) {
+	public function execute($jobList, $logger = null) {
 		if ((time() - $this->lastRun) > $this->interval) {
-			$jobList->setLastRun($this);
-			$this->run($this->argument);
+			parent::execute($jobList, $logger);
 		}
 	}
 }

tests/lib/backgroundjob/dummyjoblist.php

@@ -8,9 +8,6 @@
 
 namespace Test\BackgroundJob;
 
-class JobRun extends \Exception {
-}
-
 /**
  * Class DummyJobList
  *

tests/lib/backgroundjob/job.php

@@ -0,0 +1,59 @@
+<?php
+/**
+ * Copyright (c) 2013 Robin Appelman <icewind@owncloud.com>
+ * This file is licensed under the Affero General Public License version 3 or
+ * later.
+ * See the COPYING-README file.
+ */
+
+namespace Test\BackgroundJob;
+
+
+class TestJob extends \OC\BackgroundJob\Job {
+	private $testCase;
+
+	/**
+	 * @var callable $callback
+	 */
+	private $callback;
+
+	/**
+	 * @param Job $testCase
+	 * @param callable $callback
+	 */
+	public function __construct($testCase, $callback) {
+		$this->testCase = $testCase;
+		$this->callback = $callback;
+	}
+
+	public function run($argument) {
+		$this->testCase->markRun();
+		$callback = $this->callback;
+		$callback($argument);
+	}
+}
+
+class Job extends \PHPUnit_Framework_TestCase {
+	private $run = false;
+
+	public function setUp() {
+		$this->run = false;
+	}
+
+	public function testRemoveAfterException() {
+		$jobList = new DummyJobList();
+		$job = new TestJob($this, function () {
+			throw new \Exception();
+		});
+		$jobList->add($job);
+
+		$this->assertCount(1, $jobList->getAll());
+		$job->execute($jobList);
+		$this->assertTrue($this->run);
+		$this->assertCount(0, $jobList->getAll());
+	}
+
+	public function markRun() {
+		$this->run = true;
+	}
+}

tests/lib/backgroundjob/queuedjob.php

@@ -9,8 +9,17 @@
 namespace Test\BackgroundJob;
 
 class TestQueuedJob extends \OC\BackgroundJob\QueuedJob {
+	private $testCase;
+
+	/**
+	 * @param QueuedJob $testCase
+	 */
+	public function __construct($testCase) {
+		$this->testCase = $testCase;
+	}
+
 	public function run($argument) {
-		throw new JobRun(); //throw an exception so we can detect if this function is called
+		$this->testCase->markRun();
 	}
 }
 
@@ -24,19 +33,22 @@ class QueuedJob extends \PHPUnit_Framework_TestCase {
 	 */
 	private $job;
 
+	private $jobRun = false;
+
+	public function markRun() {
+		$this->jobRun = true;
+	}
+
 	public function setup() {
 		$this->jobList = new DummyJobList();
-		$this->job = new TestQueuedJob();
+		$this->job = new TestQueuedJob($this);
 		$this->jobList->add($this->job);
+		$this->jobRun = false;
 	}
 
 	public function testJobShouldBeRemoved() {
-		try {
-			$this->assertTrue($this->jobList->has($this->job, null));
-			$this->job->execute($this->jobList);
-			$this->fail("job should have been run");
-		} catch (JobRun $e) {
-			$this->assertFalse($this->jobList->has($this->job, null));
-		}
+		$this->assertTrue($this->jobList->has($this->job, null));
+		$this->job->execute($this->jobList);
+		$this->assertTrue($this->jobRun);
 	}
 }

tests/lib/backgroundjob/timedjob.php

@@ -9,12 +9,18 @@
 namespace Test\BackgroundJob;
 
 class TestTimedJob extends \OC\BackgroundJob\TimedJob {
-	public function __construct() {
+	private $testCase;
+
+	/**
+	 * @param TimedJob $testCase
+	 */
+	public function __construct($testCase) {
 		$this->setInterval(10);
+		$this->testCase = $testCase;
 	}
 
 	public function run($argument) {
-		throw new JobRun(); //throw an exception so we can detect if this function is called
+		$this->testCase->markRun();
 	}
 }
 
@@ -28,44 +34,37 @@ class TimedJob extends \PHPUnit_Framework_TestCase {
 	 */
 	private $job;
 
+	private $jobRun = false;
+
+	public function markRun() {
+		$this->jobRun = true;
+	}
+
 	public function setup() {
 		$this->jobList = new DummyJobList();
-		$this->job = new TestTimedJob();
+		$this->job = new TestTimedJob($this);
 		$this->jobList->add($this->job);
+		$this->jobRun = false;
 	}
 
 	public function testShouldRunAfterInterval() {
 		$this->job->setLastRun(time() - 12);
-		try {
-			$this->job->execute($this->jobList);
-			$this->fail("job should have run");
-		} catch (JobRun $e) {
-		}
-		$this->assertTrue(true);
+		$this->job->execute($this->jobList);
+		$this->assertTrue($this->jobRun);
 	}
 
 	public function testShouldNotRunWithinInterval() {
 		$this->job->setLastRun(time() - 5);
-		try {
-			$this->job->execute($this->jobList);
-		} catch (JobRun $e) {
-			$this->fail("job should not have run");
-		}
-		$this->assertTrue(true);
+		$this->job->execute($this->jobList);
+		$this->assertFalse($this->jobRun);
 	}
 
 	public function testShouldNotTwice() {
 		$this->job->setLastRun(time() - 15);
-		try {
-			$this->job->execute($this->jobList);
-			$this->fail("job should have run the first time");
-		} catch (JobRun $e) {
-			try {
-				$this->job->execute($this->jobList);
-			} catch (JobRun $e) {
-				$this->fail("job should not have run the second time");
-			}
-		}
-		$this->assertTrue(true);
+		$this->job->execute($this->jobList);
+		$this->assertTrue($this->jobRun);
+		$this->jobRun = false;
+		$this->job->execute($this->jobList);
+		$this->assertFalse($this->jobRun);
 	}
 }