diff --git a/app/code/Magento/Cron/Model/ResourceModel/Schedule.php b/app/code/Magento/Cron/Model/ResourceModel/Schedule.php index dca4e228e58d3..59974cb1a1518 100644 --- a/app/code/Magento/Cron/Model/ResourceModel/Schedule.php +++ b/app/code/Magento/Cron/Model/ResourceModel/Schedule.php @@ -23,9 +23,10 @@ public function _construct() } /** - * If job is currently in $currentStatus, set it to $newStatus - * and return true. Otherwise, return false and do not change the job. - * This method is used to implement locking for cron jobs. + * Sets new schedule status only if it's in the expected current status. + * + * If schedule is currently in $currentStatus, set it to $newStatus and + * return true. Otherwise, return false. * * @param string $scheduleId * @param string $newStatus @@ -45,4 +46,41 @@ public function trySetJobStatusAtomic($scheduleId, $newStatus, $currentStatus) } return false; } + + /** + * Sets schedule status only if no existing schedules with the same job code + * have that status. This is used to implement locking for cron jobs. + * + * If the schedule is currently in $currentStatus and there are no existing + * schedules with the same job code and $newStatus, set the schedule to + * $newStatus and return true. Otherwise, return false. + * + * @param string $scheduleId + * @param string $newStatus + * @param string $currentStatus + * @return bool + */ + public function trySetJobUniqueStatusAtomic($scheduleId, $newStatus, $currentStatus) + { + $connection = $this->getConnection(); + + $match = $connection->quoteInto('existing.job_code = current.job_code AND existing.status = ?', $newStatus); + $selectIfUnlocked = $connection->select() + ->joinLeft( + ['existing' => $this->getTable('cron_schedule')], + $match, + ['status' => new \Zend_Db_Expr($connection->quote($newStatus))] + ) + ->where('current.schedule_id = ?', $scheduleId) + ->where('current.status = ?', $currentStatus) + ->where('existing.schedule_id IS NULL'); + + $update = $connection->updateFromSelect($selectIfUnlocked, ['current' => $this->getTable('cron_schedule')]); + $result = $connection->query($update)->rowCount(); + + if ($result == 1) { + return true; + } + return false; + } } diff --git a/app/code/Magento/Cron/Model/Schedule.php b/app/code/Magento/Cron/Model/Schedule.php index b9861717b619d..2c593f6e50768 100644 --- a/app/code/Magento/Cron/Model/Schedule.php +++ b/app/code/Magento/Cron/Model/Schedule.php @@ -221,16 +221,17 @@ public function getNumeric($value) } /** - * Sets a job to STATUS_RUNNING only if it is currently in STATUS_PENDING. - * Returns true if status was changed and false otherwise. + * Lock the cron job so no other scheduled instances run simultaneously. * - * This is used to implement locking for cron jobs. + * Sets a job to STATUS_RUNNING only if it is currently in STATUS_PENDING + * and no other jobs of the same code are currently in STATUS_RUNNING. + * Returns true if status was changed and false otherwise. * * @return boolean */ public function tryLockJob() { - if ($this->_getResource()->trySetJobStatusAtomic( + if ($this->_getResource()->trySetJobUniqueStatusAtomic( $this->getId(), self::STATUS_RUNNING, self::STATUS_PENDING diff --git a/app/code/Magento/Cron/Test/Unit/Model/ScheduleTest.php b/app/code/Magento/Cron/Test/Unit/Model/ScheduleTest.php index 91e70e9bfb642..69cb3c8c71e06 100644 --- a/app/code/Magento/Cron/Test/Unit/Model/ScheduleTest.php +++ b/app/code/Magento/Cron/Test/Unit/Model/ScheduleTest.php @@ -23,7 +23,7 @@ protected function setUp() $this->resourceJobMock = $this->getMockBuilder('Magento\Cron\Model\ResourceModel\Schedule') ->disableOriginalConstructor() - ->setMethods(['trySetJobStatusAtomic', '__wakeup', 'getIdFieldName']) + ->setMethods(['trySetJobUniqueStatusAtomic', '__wakeup', 'getIdFieldName']) ->getMockForAbstractClass(); $this->resourceJobMock->expects($this->any()) @@ -336,7 +336,7 @@ public function testTryLockJobSuccess() $scheduleId = 1; $this->resourceJobMock->expects($this->once()) - ->method('trySetJobStatusAtomic') + ->method('trySetJobUniqueStatusAtomic') ->with($scheduleId, Schedule::STATUS_RUNNING, Schedule::STATUS_PENDING) ->will($this->returnValue(true)); @@ -360,7 +360,7 @@ public function testTryLockJobFailure() $scheduleId = 1; $this->resourceJobMock->expects($this->once()) - ->method('trySetJobStatusAtomic') + ->method('trySetJobUniqueStatusAtomic') ->with($scheduleId, Schedule::STATUS_RUNNING, Schedule::STATUS_PENDING) ->will($this->returnValue(false)); diff --git a/dev/tests/integration/testsuite/Magento/Cron/Model/ScheduleTest.php b/dev/tests/integration/testsuite/Magento/Cron/Model/ScheduleTest.php new file mode 100644 index 0000000000000..41693460ec7b3 --- /dev/null +++ b/dev/tests/integration/testsuite/Magento/Cron/Model/ScheduleTest.php @@ -0,0 +1,99 @@ +dateTime = Bootstrap::getObjectManager()->create(DateTime::class); + $this->scheduleFactory = Bootstrap::getObjectManager()->create(ScheduleFactory::class); + } + + /** + * If there are no currently locked jobs, locking one of them should succeed + */ + public function testTryLockJobNoLockedJobsSucceeds() + { + for ($i = 1; $i < 6; $i++) { + $this->createSchedule("test_job", Schedule::STATUS_PENDING, 60 * $i); + } + $schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING); + + $this->assertTrue($schedule->tryLockJob()); + } + + /** + * If the job is already locked, attempting to lock it again should fail + */ + public function testTryLockJobAlreadyLockedFails() + { + $schedule = $this->createSchedule("test_job", Schedule::STATUS_RUNNING); + + $this->assertFalse($schedule->tryLockJob()); + } + + /** + * If there's a job already locked, should not be able to lock another job + */ + public function testTryLockJobOtherLockedFails() + { + $this->createSchedule("test_job", Schedule::STATUS_RUNNING); + $schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING, 60); + + $this->assertFalse($schedule->tryLockJob()); + } + + /** + * Should be able to lock a job if a job with a different code is locked + */ + public function testTryLockJobDifferentJobLocked() + { + $this->createSchedule("test_job_other", Schedule::STATUS_RUNNING); + $schedule = $this->createSchedule("test_job", Schedule::STATUS_PENDING); + + $this->assertTrue($schedule->tryLockJob()); + } + + /** + * Creates a schedule with the given job code, status, and schedule time offset + * + * @param string $jobCode + * @param string $status + * @param int $timeOffset + * @return Schedule + */ + private function createSchedule($jobCode, $status, $timeOffset = 0) + { + $schedule = $this->scheduleFactory->create() + ->setCronExpr("* * * * *") + ->setJobCode($jobCode) + ->setStatus($status) + ->setCreatedAt(strftime('%Y-%m-%d %H:%M:%S', $this->dateTime->gmtTimestamp())) + ->setScheduledAt(strftime('%Y-%m-%d %H:%M', $this->dateTime->gmtTimestamp() + $timeOffset)); + $schedule->save(); + + return $schedule; + } +}