Skip to content

Commit be36e5a

Browse files
cparticacpartica
committed
Merge remote-tracking branch 'origin/MAGETWO-69629-CronJob-Locking' into Okapis-develop-pr
2 parents 82416d7 + e110f1b commit be36e5a

File tree

4 files changed

+141
-5
lines changed

4 files changed

+141
-5
lines changed

‎app/code/Magento/Cron/Model/ResourceModel/Schedule.php

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,4 +45,40 @@ public function trySetJobStatusAtomic($scheduleId, $newStatus, $currentStatus)
4545
}
4646
return false;
4747
}
48+
49+
/**
50+
* If job is currently in $currentStatus and there are no existing jobs
51+
* with $newStatus, set it to $newStatus and return true. Otherwise,
52+
* return false and do not change the job.
53+
* This method is used to implement locking for cron jobs that share a
54+
* job code.
55+
*
56+
* @param string $scheduleId
57+
* @param string $newStatus
58+
* @param string $currentStatus
59+
* @return bool
60+
*/
61+
public function trySetJobUniqueStatusAtomic($scheduleId, $newStatus, $currentStatus)
62+
{
63+
$connection = $this->getConnection();
64+
65+
$match = $connection->quoteInto('existing.job_code = current.job_code AND existing.status = ?', $newStatus);
66+
$selectIfUnlocked = $connection->select()
67+
->joinLeft(
68+
['existing' => $this->getTable('cron_schedule')],
69+
$match,
70+
['status' => new \Zend_Db_Expr($connection->quote($newStatus))]
71+
)
72+
->where('current.schedule_id = ?', $scheduleId)
73+
->where('current.status = ?', $currentStatus)
74+
->where('existing.schedule_id IS NULL');
75+
76+
$update = $connection->updateFromSelect($selectIfUnlocked, ['current' => $this->getTable('cron_schedule')]);
77+
$result = $connection->query($update)->rowCount();
78+
79+
if ($result == 1) {
80+
return true;
81+
}
82+
return false;
83+
}
4884
}

‎app/code/Magento/Cron/Model/Schedule.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,8 @@ public function getNumeric($value)
221221
}
222222

223223
/**
224-
* Sets a job to STATUS_RUNNING only if it is currently in STATUS_PENDING.
224+
* Sets a job to STATUS_RUNNING only if it is currently in STATUS_PENDING
225+
* and no other jobs of the same code are currently in STATUS_RUNNING.
225226
* Returns true if status was changed and false otherwise.
226227
*
227228
* This is used to implement locking for cron jobs.
@@ -230,7 +231,7 @@ public function getNumeric($value)
230231
*/
231232
public function tryLockJob()
232233
{
233-
if ($this->_getResource()->trySetJobStatusAtomic(
234+
if ($this->_getResource()->trySetJobUniqueStatusAtomic(
234235
$this->getId(),
235236
self::STATUS_RUNNING,
236237
self::STATUS_PENDING

‎app/code/Magento/Cron/Test/Unit/Model/ScheduleTest.php

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ protected function setUp()
2323

2424
$this->resourceJobMock = $this->getMockBuilder(\Magento\Cron\Model\ResourceModel\Schedule::class)
2525
->disableOriginalConstructor()
26-
->setMethods(['trySetJobStatusAtomic', '__wakeup', 'getIdFieldName'])
26+
->setMethods(['trySetJobUniqueStatusAtomic', '__wakeup', 'getIdFieldName'])
2727
->getMockForAbstractClass();
2828

2929
$this->resourceJobMock->expects($this->any())
@@ -336,7 +336,7 @@ public function testTryLockJobSuccess()
336336
$scheduleId = 1;
337337

338338
$this->resourceJobMock->expects($this->once())
339-
->method('trySetJobStatusAtomic')
339+
->method('trySetJobUniqueStatusAtomic')
340340
->with($scheduleId, Schedule::STATUS_RUNNING, Schedule::STATUS_PENDING)
341341
->will($this->returnValue(true));
342342

@@ -360,7 +360,7 @@ public function testTryLockJobFailure()
360360
$scheduleId = 1;
361361

362362
$this->resourceJobMock->expects($this->once())
363-
->method('trySetJobStatusAtomic')
363+
->method('trySetJobUniqueStatusAtomic')
364364
->with($scheduleId, Schedule::STATUS_RUNNING, Schedule::STATUS_PENDING)
365365
->will($this->returnValue(false));
366366

‎dev/tests/integration/testsuite/Magento/Cron/Model/ScheduleTest.php

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
<?php
2+
/**
3+
* Copyright © Magento, Inc. All rights reserved.
4+
* See COPYING.txt for license details.
5+
*/
6+
namespace Magento\Cron\Model;
7+
8+
use Magento\Framework\Stdlib\DateTime\DateTime;
9+
use \Magento\TestFramework\Helper\Bootstrap;
10+
11+
/**
12+
* Test \Magento\Cron\Model\Schedule
13+
*
14+
* @magentoDbIsolation enabled
15+
*/
16+
class ScheduleTest extends \PHPUnit_Framework_TestCase
17+
{
18+
/**
19+
* @var ScheduleFactory
20+
*/
21+
private $scheduleFactory;
22+
23+
/**
24+
* @var DateTime
25+
*/
26+
protected $dateTime;
27+
28+
public function setUp()
29+
{
30+
$this->dateTime = Bootstrap::getObjectManager()->create(DateTime::class);
31+
$this->scheduleFactory = Bootstrap::getObjectManager()->create(ScheduleFactory::class);
32+
}
33+
34+
/**
35+
* If there are no currently locked jobs, locking one of them should succeed
36+
*/
37+
public function testTryLockJobNoLockedJobsSucceeds()
38+
{
39+
for ($i = 1; $i < 6; $i++) {
40+
$this->createSchedule("test_job", Schedule::STATUS_PENDING, 60 * $i);
41+
}
42+
$schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING);
43+
44+
$this->assertTrue($schedule->tryLockJob());
45+
}
46+
47+
/**
48+
* If the job is already locked, attempting to lock it again should fail
49+
*/
50+
public function testTryLockJobAlreadyLockedFails()
51+
{
52+
$schedule = $this->createSchedule("test_job", Schedule::STATUS_RUNNING);
53+
54+
$this->assertFalse($schedule->tryLockJob());
55+
}
56+
57+
/**
58+
* If there's a job already locked, should not be able to lock another job
59+
*/
60+
public function testTryLockJobOtherLockedFails()
61+
{
62+
$this->createSchedule("test_job", Schedule::STATUS_RUNNING);
63+
$schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING, 60);
64+
65+
$this->assertFalse($schedule->tryLockJob());
66+
}
67+
68+
/**
69+
* Should be able to lock a job if a job with a different code is locked
70+
*/
71+
public function testTryLockJobDifferentJobLocked()
72+
{
73+
$this->createSchedule("test_job_other", Schedule::STATUS_RUNNING);
74+
$schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING);
75+
76+
$this->assertTrue($schedule->tryLockJob());
77+
}
78+
79+
/**
80+
* Creates a schedule with the given job code, status, and schedule time offset
81+
*
82+
* @param string $jobCode
83+
* @param string $status
84+
* @param int $timeOffset
85+
* @return Schedule
86+
*/
87+
private function createSchedule($jobCode, $status, $timeOffset = 0)
88+
{
89+
$schedule = $this->scheduleFactory->create()
90+
->setCronExpr("* * * * *")
91+
->setJobCode($jobCode)
92+
->setStatus($status)
93+
->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', $this->dateTime->gmtTimestamp()))
94+
->setScheduledAt(strftime('%Y-%m-%d %H:%M', $this->dateTime->gmtTimestamp() + $timeOffset));
95+
$schedule->save();
96+
97+
return $schedule;
98+
}
99+
}

0 commit comments

Comments
 (0)