From 35bb6f7e3a4df325e594e1d206520e89014f2a42 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Dec 2013 13:41:47 +0100 Subject: [PATCH 1/4] Catch exceptions from background jobs and log them --- cron.php | 6 ++++-- lib/private/backgroundjob/job.php | 22 ++++++++++++++++++++-- lib/private/backgroundjob/queuedjob.php | 5 +++-- lib/private/backgroundjob/timedjob.php | 6 +++--- 4 files changed, 30 insertions(+), 9 deletions(-) diff --git a/cron.php b/cron.php index 8e1a3376d5..0d2c07b2d9 100644 --- a/cron.php +++ b/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(); } diff --git a/lib/private/backgroundjob/job.php b/lib/private/backgroundjob/job.php index 49fbffbd68..2561659750 100644 --- a/lib/private/backgroundjob/job.php +++ b/lib/private/backgroundjob/job.php @@ -9,16 +9,34 @@ 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()); + } + } } abstract protected function run($argument); diff --git a/lib/private/backgroundjob/queuedjob.php b/lib/private/backgroundjob/queuedjob.php index 1714182820..799eac4784 100644 --- a/lib/private/backgroundjob/queuedjob.php +++ b/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); } } diff --git a/lib/private/backgroundjob/timedjob.php b/lib/private/backgroundjob/timedjob.php index ae9f33505a..09e05f1d84 100644 --- a/lib/private/backgroundjob/timedjob.php +++ b/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); } } } From 4f4ad72460f8ef299cd1235b5774c5f121cca430 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Mon, 2 Dec 2013 13:43:26 +0100 Subject: [PATCH 2/4] remove background jobs if they are failing --- lib/private/backgroundjob/job.php | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/private/backgroundjob/job.php b/lib/private/backgroundjob/job.php index 2561659750..92bd0f8fdb 100644 --- a/lib/private/backgroundjob/job.php +++ b/lib/private/backgroundjob/job.php @@ -36,6 +36,7 @@ abstract class Job { if ($logger) { $logger->error('Error while running background job: ' . $e->getMessage()); } + $jobList->remove($this, $this->argument); } } From 3fa11bd426bbb68c02bd98519c7f0f1451257dc7 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Wed, 4 Dec 2013 16:28:27 +0100 Subject: [PATCH 3/4] Dont use exceptions for the backgroundjob test cases --- tests/lib/backgroundjob/queuedjob.php | 30 +++++++++++----- tests/lib/backgroundjob/timedjob.php | 51 +++++++++++++-------------- 2 files changed, 46 insertions(+), 35 deletions(-) diff --git a/tests/lib/backgroundjob/queuedjob.php b/tests/lib/backgroundjob/queuedjob.php index 1d373473cd..19c1b28a50 100644 --- a/tests/lib/backgroundjob/queuedjob.php +++ b/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); } } diff --git a/tests/lib/backgroundjob/timedjob.php b/tests/lib/backgroundjob/timedjob.php index f3c3eb4d0d..646a2607ef 100644 --- a/tests/lib/backgroundjob/timedjob.php +++ b/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); } } From 2ff1bdaba3292cc5baff71d8d79674bbf0b69fc8 Mon Sep 17 00:00:00 2001 From: Robin Appelman Date: Thu, 5 Dec 2013 13:29:35 +0100 Subject: [PATCH 4/4] add test case for removing background jobs that are throwing exceptions --- tests/lib/backgroundjob/dummyjoblist.php | 3 -- tests/lib/backgroundjob/job.php | 59 ++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) create mode 100644 tests/lib/backgroundjob/job.php diff --git a/tests/lib/backgroundjob/dummyjoblist.php b/tests/lib/backgroundjob/dummyjoblist.php index d91d6b344b..e1579c273b 100644 --- a/tests/lib/backgroundjob/dummyjoblist.php +++ b/tests/lib/backgroundjob/dummyjoblist.php @@ -8,9 +8,6 @@ namespace Test\BackgroundJob; -class JobRun extends \Exception { -} - /** * Class DummyJobList * diff --git a/tests/lib/backgroundjob/job.php b/tests/lib/backgroundjob/job.php new file mode 100644 index 0000000000..7d66fa772d --- /dev/null +++ b/tests/lib/backgroundjob/job.php @@ -0,0 +1,59 @@ + + * 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; + } +}