Merge pull request magento#1227 from magento-okapis/Okapis-develop-pr

[okapis] MAGETWO-69629: Deadlock Error While indexing Category products

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;
+    }
 }

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

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));
 

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

@@ -0,0 +1,99 @@
+<?php
+/**
+ * Copyright © Magento, Inc. All rights reserved.
+ * See COPYING.txt for license details.
+ */
+namespace Magento\Cron\Model;
+
+use Magento\Framework\Stdlib\DateTime\DateTime;
+use \Magento\TestFramework\Helper\Bootstrap;
+
+/**
+ * Test \Magento\Cron\Model\Schedule
+ *
+ * @magentoDbIsolation enabled
+ */
+class ScheduleTest extends \PHPUnit_Framework_TestCase
+{
+    /**
+     * @var ScheduleFactory
+     */
+    private $scheduleFactory;
+
+    /**
+     * @var DateTime
+     */
+    protected $dateTime;
+
+    public function setUp()
+    {
+        $this->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;
+    }
+}