-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Prevent running again already running cron group #12497
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
c959393
702c93a
fcda71d
016387c
23cb078
b69945b
071df9f
7a88a9a
2972e21
4ad9c5d
1e546c8
47759c8
dd0b861
d1b56b2
c2e5520
7992e1e
2a7099e
79310b9
229476f
d12581e
bf8409b
209b2bd
c8be788
83b17fe
79d60cb
50dcc3d
175229e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -56,6 +56,11 @@ class ProcessCronQueueObserver implements ObserverInterface | |
*/ | ||
const SECONDS_IN_MINUTE = 60; | ||
|
||
/** | ||
* How long to wait for cron group to become unlocked | ||
*/ | ||
const LOCK_TIMEOUT = 5; | ||
|
||
/** | ||
* @var \Magento\Cron\Model\ResourceModel\Schedule\Collection | ||
*/ | ||
|
@@ -116,6 +121,11 @@ class ProcessCronQueueObserver implements ObserverInterface | |
*/ | ||
private $state; | ||
|
||
/** | ||
* @var \Magento\Framework\Lock\LockManagerInterface | ||
*/ | ||
private $lockManager; | ||
|
||
/** | ||
* @var array | ||
*/ | ||
|
@@ -138,6 +148,7 @@ class ProcessCronQueueObserver implements ObserverInterface | |
* @param \Magento\Framework\Process\PhpExecutableFinderFactory $phpExecutableFinderFactory | ||
* @param \Psr\Log\LoggerInterface $logger | ||
* @param \Magento\Framework\App\State $state | ||
* @param \Magento\Framework\Lock\LockManagerInterface $lockManager | ||
* @SuppressWarnings(PHPMD.ExcessiveParameterList) | ||
*/ | ||
public function __construct( | ||
|
@@ -151,7 +162,8 @@ public function __construct( | |
\Magento\Framework\Stdlib\DateTime\DateTime $dateTime, | ||
\Magento\Framework\Process\PhpExecutableFinderFactory $phpExecutableFinderFactory, | ||
\Psr\Log\LoggerInterface $logger, | ||
\Magento\Framework\App\State $state | ||
\Magento\Framework\App\State $state, | ||
\Magento\Framework\Lock\LockManagerInterface $lockManager | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't BC be preserved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @adrian-martinez-interactiv4, We treat Observers and Plugins as internal implementation and don't try to preserve BC for them |
||
) { | ||
$this->_objectManager = $objectManager; | ||
$this->_scheduleFactory = $scheduleFactory; | ||
|
@@ -164,6 +176,7 @@ public function __construct( | |
$this->phpExecutableFinder = $phpExecutableFinderFactory->create(); | ||
$this->logger = $logger; | ||
$this->state = $state; | ||
$this->lockManager = $lockManager; | ||
} | ||
|
||
/** | ||
|
@@ -186,8 +199,6 @@ public function execute(\Magento\Framework\Event\Observer $observer) | |
$phpPath = $this->phpExecutableFinder->find() ?: 'php'; | ||
|
||
foreach ($jobGroupsRoot as $groupId => $jobsRoot) { | ||
$this->_cleanup($groupId); | ||
$this->_generate($groupId); | ||
if ($this->_request->getParam('group') !== null | ||
&& $this->_request->getParam('group') !== '\'' . ($groupId) . '\'' | ||
&& $this->_request->getParam('group') !== $groupId | ||
|
@@ -211,6 +222,19 @@ public function execute(\Magento\Framework\Event\Observer $observer) | |
continue; | ||
} | ||
|
||
if (!$this->lockManager->setLock($groupId, self::LOCK_TIMEOUT)) { | ||
$this->logger->warning( | ||
sprintf( | ||
"Could not acquire lock for cron group: %s, skipping run", | ||
$groupId | ||
) | ||
); | ||
continue; | ||
} | ||
|
||
$this->_cleanup($groupId); | ||
$this->_generate($groupId); | ||
|
||
/** @var \Magento\Cron\Model\Schedule $schedule */ | ||
foreach ($pendingJobs as $schedule) { | ||
$jobConfig = isset($jobsRoot[$schedule->getJobCode()]) ? $jobsRoot[$schedule->getJobCode()] : null; | ||
|
@@ -247,6 +271,8 @@ public function execute(\Magento\Framework\Event\Observer $observer) | |
} | ||
$schedule->save(); | ||
} | ||
|
||
$this->lockManager->releaseLock($groupId); | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
|
||
namespace Magento\Framework\lock\Backend; | ||
|
||
class Database implements \Magento\Framework\Lock\LockManagerInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would be great to cover this class with unit and integration tests. Could you add them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, I'll work on implementing some test cases for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test cases added. |
||
{ | ||
/** @var \Magento\Framework\App\ResourceConnection */ | ||
private $resource; | ||
|
||
public function __construct( | ||
\Magento\Framework\App\ResourceConnection $resource | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you import this resource connection, it will be more readable there. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
) | ||
{ | ||
$this->resource = $resource; | ||
} | ||
|
||
/** | ||
* Sets a lock for name | ||
* | ||
* @param string $name lock name | ||
* @return bool | ||
*/ | ||
public function setLock($name, $timeout = -1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As magento 2.2 and later support php 7+ only, could you scecify types for these variables? Same for other methods in this file + interface. Also would be great to add declare strict types for new file. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another thing:
I think it should be checked in order to prevent mysql errors there There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Implemented scalar type hints. |
||
{ | ||
return (bool)$this->resource->getConnection()->query("SELECT GET_LOCK(?, ?);", array((string)$name, (int)$timeout)) | ||
->fetchColumn(); | ||
} | ||
|
||
/** | ||
* Releases a lock for name | ||
* | ||
* @param string $name lock name | ||
* @return bool | ||
*/ | ||
public function releaseLock($name) | ||
{ | ||
return (bool)$this->resource->getConnection()->query("SELECT RELEASE_LOCK(?);", array((string)$name))->fetchColumn(); | ||
} | ||
|
||
/** | ||
* Tests of lock is set for name | ||
* | ||
* @param string $name lock name | ||
* @return bool | ||
*/ | ||
public function isLocked($name) | ||
{ | ||
return (bool)$this->resource->getConnection()->query("SELECT IS_USED_LOCK(?);", array((string)$name))->fetchColumn(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
namespace Magento\Framework\Lock; | ||
|
||
/** | ||
* Interface of a lock manager | ||
* | ||
* @api | ||
*/ | ||
interface LockManagerInterface | ||
{ | ||
/** | ||
* Sets a lock for name | ||
* | ||
* @param string $name lock name | ||
* @param int $timeout Timeout in seconds, negative value means infinite timeout | ||
* @return bool | ||
*/ | ||
public function setLock($name, $timeout = -1); | ||
|
||
/** | ||
* Releases a lock for name | ||
* | ||
* @param string $name lock name | ||
* @return bool | ||
* @api | ||
*/ | ||
public function releaseLock($name); | ||
|
||
/** | ||
* Tests of lock is set for name | ||
* | ||
* @param string $name lock name | ||
* @return bool | ||
* @api | ||
*/ | ||
public function isLocked($name); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not hard-code timeout. Some processes can take more than 5min based on the amount of the processed data. The easiest solution would be to make it configurable (either in Admin Panel or in config.php). Better solution could be to adjust automatically (e.g., make each process output something and if there is no output consider it hung and release the lock), but it might be a more complex solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is actually five seconds, after that we give up and continue to the next group.
Reasoning being: cron:run is triggered from system crontab every minute, so next check will happen within so small period, that it does not make sense to wait for a long time. I suppose this could be reduced to even one or zero seconds (skip group immediately if lock could not be taken).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. Thanks. I think it's ok to leave it as is (5s).