Skip to content

Commit 2992ee2

Browse files
cparticacpartica
authored
Merge pull request #1227 from magento-okapis/Okapis-develop-pr
[okapis] MAGETWO-69629: Deadlock Error While indexing Category products
2 parents 3cfa32b + 3d1db43 commit 2992ee2

File tree

4 files changed

+148
-10
lines changed

4 files changed

+148
-10
lines changed

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

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,10 @@ public function _construct()
2323
}
2424

2525
/**
26-
* If job is currently in $currentStatus, set it to $newStatus
27-
* and return true. Otherwise, return false and do not change the job.
28-
* This method is used to implement locking for cron jobs.
26+
* Sets new schedule status only if it's in the expected current status.
27+
*
28+
* If schedule is currently in $currentStatus, set it to $newStatus and
29+
* return true. Otherwise, return false.
2930
*
3031
* @param string $scheduleId
3132
* @param string $newStatus
@@ -45,4 +46,41 @@ public function trySetJobStatusAtomic($scheduleId, $newStatus, $currentStatus)
4546
}
4647
return false;
4748
}
49+
50+
/**
51+
* Sets schedule status only if no existing schedules with the same job code
52+
* have that status. This is used to implement locking for cron jobs.
53+
*
54+
* If the schedule is currently in $currentStatus and there are no existing
55+
* schedules with the same job code and $newStatus, set the schedule to
56+
* $newStatus and return true. Otherwise, return false.
57+
*
58+
* @param string $scheduleId
59+
* @param string $newStatus
60+
* @param string $currentStatus
61+
* @return bool
62+
*/
63+
public function trySetJobUniqueStatusAtomic($scheduleId, $newStatus, $currentStatus)
64+
{
65+
$connection = $this->getConnection();
66+
67+
$match = $connection->quoteInto('existing.job_code = current.job_code AND existing.status = ?', $newStatus);
68+
$selectIfUnlocked = $connection->select()
69+
->joinLeft(
70+
['existing' => $this->getTable('cron_schedule')],
71+
$match,
72+
['status' => new \Zend_Db_Expr($connection->quote($newStatus))]
73+
)
74+
->where('current.schedule_id = ?', $scheduleId)
75+
->where('current.status = ?', $currentStatus)
76+
->where('existing.schedule_id IS NULL');
77+
78+
$update = $connection->updateFromSelect($selectIfUnlocked, ['current' => $this->getTable('cron_schedule')]);
79+
$result = $connection->query($update)->rowCount();
80+
81+
if ($result == 1) {
82+
return true;
83+
}
84+
return false;
85+
}
4886
}

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -221,16 +221,17 @@ public function getNumeric($value)
221221
}
222222

223223
/**
224-
* Sets a job to STATUS_RUNNING only if it is currently in STATUS_PENDING.
225-
* Returns true if status was changed and false otherwise.
224+
* Lock the cron job so no other scheduled instances run simultaneously.
226225
*
227-
* This is used to implement locking for cron jobs.
226+
* Sets a job to STATUS_RUNNING only if it is currently in STATUS_PENDING
227+
* and no other jobs of the same code are currently in STATUS_RUNNING.
228+
* Returns true if status was changed and false otherwise.
228229
*
229230
* @return boolean
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)