Skip to content

Commit 850db06

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #39612 from nextcloud/backport/39473/stable27
[stable27] fix(IParallelAwareJob): Check for other reserved jobs before setting new ones as reserved
2 parents 6d9cf9e + 070533e commit 850db06

File tree

5 files changed

+111
-71
lines changed

5 files changed

+111
-71
lines changed

‎lib/private/BackgroundJob/JobList.php‎

Lines changed: 23 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
use OCP\AutoloadNotAllowedException;
3636
use OCP\BackgroundJob\IJob;
3737
use OCP\BackgroundJob\IJobList;
38+
use OCP\BackgroundJob\IParallelAwareJob;
3839
use OCP\DB\Exception;
3940
use OCP\DB\QueryBuilder\IQueryBuilder;
4041
use OCP\IConfig;
@@ -218,19 +219,33 @@ public function getNext(bool $onlyTimeSensitive = false): ?IJob {
218219
$query->andWhere($query->expr()->eq('time_sensitive', $query->createNamedParameter(IJob::TIME_SENSITIVE, IQueryBuilder::PARAM_INT)));
219220
}
220221

221-
$update = $this->connection->getQueryBuilder();
222-
$update->update('jobs')
223-
->set('reserved_at', $update->createNamedParameter($this->timeFactory->getTime()))
224-
->set('last_checked', $update->createNamedParameter($this->timeFactory->getTime()))
225-
->where($update->expr()->eq('id', $update->createParameter('jobid')))
226-
->andWhere($update->expr()->eq('reserved_at', $update->createParameter('reserved_at')))
227-
->andWhere($update->expr()->eq('last_checked', $update->createParameter('last_checked')));
228-
229222
$result = $query->executeQuery();
230223
$row = $result->fetch();
231224
$result->closeCursor();
232225

233226
if ($row) {
227+
$job = $this->buildJob($row);
228+
229+
if ($job instanceof IParallelAwareJob && !$job->getAllowParallelRuns() && $this->hasReservedJob(get_class($job))) {
230+
$this->logger->debug('Skipping ' . get_class($job) . ' job with ID ' . $job->getId() . ' because another job with the same class is already running', ['app' => 'cron']);
231+
232+
$update = $this->connection->getQueryBuilder();
233+
$update->update('jobs')
234+
->set('last_checked', $update->createNamedParameter($this->timeFactory->getTime() + 1))
235+
->where($update->expr()->eq('id', $update->createParameter('jobid')));
236+
$update->setParameter('jobid', $row['id']);
237+
$update->executeStatement();
238+
239+
return $this->getNext($onlyTimeSensitive);
240+
}
241+
242+
$update = $this->connection->getQueryBuilder();
243+
$update->update('jobs')
244+
->set('reserved_at', $update->createNamedParameter($this->timeFactory->getTime()))
245+
->set('last_checked', $update->createNamedParameter($this->timeFactory->getTime()))
246+
->where($update->expr()->eq('id', $update->createParameter('jobid')))
247+
->andWhere($update->expr()->eq('reserved_at', $update->createParameter('reserved_at')))
248+
->andWhere($update->expr()->eq('last_checked', $update->createParameter('last_checked')));
234249
$update->setParameter('jobid', $row['id']);
235250
$update->setParameter('reserved_at', $row['reserved_at']);
236251
$update->setParameter('last_checked', $row['last_checked']);
@@ -240,7 +255,6 @@ public function getNext(bool $onlyTimeSensitive = false): ?IJob {
240255
// Background job already executed elsewhere, try again.
241256
return $this->getNext($onlyTimeSensitive);
242257
}
243-
$job = $this->buildJob($row);
244258

245259
if ($job === null) {
246260
// set the last_checked to 12h in the future to not check failing jobs all over again

‎lib/public/BackgroundJob/Job.php‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -75,11 +75,6 @@ public function start(IJobList $jobList): void {
7575
$jobList->setLastRun($this);
7676
$logger = $this->logger ?? \OCP\Server::get(LoggerInterface::class);
7777

78-
if (!$this->getAllowParallelRuns() && $jobList->hasReservedJob(get_class($this))) {
79-
$logger->debug('Skipping ' . get_class($this) . ' job with ID ' . $this->getId() . ' because another job with the same class is already running', ['app' => 'cron']);
80-
return;
81-
}
82-
8378
try {
8479
$jobStartTime = $this->time->getTime();
8580
$logger->debug('Run ' . get_class($this) . ' job with ID ' . $this->getId(), ['app' => 'cron']);

‎tests/lib/BackgroundJob/JobListTest.php‎

Lines changed: 51 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -250,18 +250,64 @@ public function testSetLastRun() {
250250

251251
public function testHasReservedJobs() {
252252
$this->clearJobsList();
253+
254+
$this->timeFactory->expects($this->atLeastOnce())
255+
->method('getTime')
256+
->willReturn(123456789);
257+
253258
$job = new TestJob($this->timeFactory, $this, function () {
254-
$this->assertTrue($this->instance->hasReservedJob());
255-
$this->assertTrue($this->instance->hasReservedJob(TestJob::class));
256259
});
257-
$this->instance->add($job);
260+
261+
$job2 = new TestJob($this->timeFactory, $this, function () {
262+
});
263+
264+
$this->instance->add($job, 1);
265+
$this->instance->add($job2, 2);
266+
267+
$this->assertCount(2, iterator_to_array($this->instance->getJobsIterator(null, 10, 0)));
258268

259269
$this->assertFalse($this->instance->hasReservedJob());
260270
$this->assertFalse($this->instance->hasReservedJob(TestJob::class));
261271

262-
$job->start($this->instance);
272+
$job = $this->instance->getNext();
273+
$this->assertNotNull($job);
274+
$this->assertTrue($this->instance->hasReservedJob());
275+
$this->assertTrue($this->instance->hasReservedJob(TestJob::class));
276+
$job = $this->instance->getNext();
277+
$this->assertNotNull($job);
278+
$this->assertTrue($this->instance->hasReservedJob());
279+
$this->assertTrue($this->instance->hasReservedJob(TestJob::class));
280+
}
263281

264-
$this->assertTrue($this->ran);
282+
public function testHasReservedJobsAndParallelAwareJob() {
283+
$this->clearJobsList();
284+
285+
$this->timeFactory->expects($this->atLeastOnce())
286+
->method('getTime')
287+
->willReturnCallback(function () use (&$time) {
288+
return time();
289+
});
290+
291+
$job = new TestParallelAwareJob($this->timeFactory, $this, function () {
292+
});
293+
294+
$job2 = new TestParallelAwareJob($this->timeFactory, $this, function () {
295+
});
296+
297+
$this->instance->add($job, 1);
298+
$this->instance->add($job2, 2);
299+
300+
$this->assertCount(2, iterator_to_array($this->instance->getJobsIterator(null, 10, 0)));
301+
302+
$this->assertFalse($this->instance->hasReservedJob());
303+
$this->assertFalse($this->instance->hasReservedJob(TestParallelAwareJob::class));
304+
305+
$job = $this->instance->getNext();
306+
$this->assertNotNull($job);
307+
$this->assertTrue($this->instance->hasReservedJob());
308+
$this->assertTrue($this->instance->hasReservedJob(TestParallelAwareJob::class));
309+
$job = $this->instance->getNext();
310+
$this->assertNull($job); // Job doesn't allow parallel runs
265311
}
266312

267313
public function markRun() {

‎tests/lib/BackgroundJob/JobTest.php‎

Lines changed: 0 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -61,58 +61,6 @@ public function testRemoveAfterError() {
6161
$this->assertCount(1, $jobList->getAll());
6262
}
6363

64-
public function testDisallowParallelRunsWithNoOtherJobs() {
65-
$jobList = new DummyJobList();
66-
$job = new TestJob($this->timeFactory, $this, function () {
67-
});
68-
$job->setAllowParallelRuns(false);
69-
$jobList->add($job);
70-
71-
$jobList->setHasReservedJob(null, false);
72-
$jobList->setHasReservedJob(TestJob::class, false);
73-
$job->start($jobList);
74-
$this->assertTrue($this->run);
75-
}
76-
77-
public function testAllowParallelRunsWithNoOtherJobs() {
78-
$jobList = new DummyJobList();
79-
$job = new TestJob($this->timeFactory, $this, function () {
80-
});
81-
$job->setAllowParallelRuns(true);
82-
$jobList->add($job);
83-
84-
$jobList->setHasReservedJob(null, false);
85-
$jobList->setHasReservedJob(TestJob::class, false);
86-
$job->start($jobList);
87-
$this->assertTrue($this->run);
88-
}
89-
90-
public function testAllowParallelRunsWithOtherJobs() {
91-
$jobList = new DummyJobList();
92-
$job = new TestJob($this->timeFactory, $this, function () {
93-
});
94-
$job->setAllowParallelRuns(true);
95-
$jobList->add($job);
96-
97-
$jobList->setHasReservedJob(null, true);
98-
$jobList->setHasReservedJob(TestJob::class, true);
99-
$job->start($jobList);
100-
$this->assertTrue($this->run);
101-
}
102-
103-
public function testDisallowParallelRunsWithOtherJobs() {
104-
$jobList = new DummyJobList();
105-
$job = new TestJob($this->timeFactory, $this, function () {
106-
});
107-
$job->setAllowParallelRuns(false);
108-
$jobList->add($job);
109-
110-
$jobList->setHasReservedJob(null, true);
111-
$jobList->setHasReservedJob(TestJob::class, true);
112-
$job->start($jobList);
113-
$this->assertFalse($this->run);
114-
}
115-
11664
public function markRun() {
11765
$this->run = true;
11866
}

‎tests/lib/BackgroundJob/TestParallelAwareJob.php‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?php
2+
/**
3+
* Copyright (c) 2014 Robin Appelman <icewind@owncloud.com>
4+
* This file is licensed under the Affero General Public License version 3 or
5+
* later.
6+
* See the COPYING-README file.
7+
*/
8+
9+
namespace Test\BackgroundJob;
10+
11+
use OCP\AppFramework\Utility\ITimeFactory;
12+
13+
class TestParallelAwareJob extends \OCP\BackgroundJob\Job {
14+
private $testCase;
15+
16+
/**
17+
* @var callable $callback
18+
*/
19+
private $callback;
20+
21+
/**
22+
* @param JobTest $testCase
23+
* @param callable $callback
24+
*/
25+
public function __construct(ITimeFactory $time = null, $testCase = null, $callback = null) {
26+
parent::__construct($time ?? \OC::$server->get(ITimeFactory::class));
27+
$this->setAllowParallelRuns(false);
28+
$this->testCase = $testCase;
29+
$this->callback = $callback;
30+
}
31+
32+
public function run($argument) {
33+
$this->testCase->markRun();
34+
$callback = $this->callback;
35+
$callback($argument);
36+
}
37+
}

0 commit comments

Comments
 (0)