A recurring issue is that the Drush queue worker doesn't match the logic for queues processed by cron, leading to subtle bugs.

https://github.com/drush-ops/drush/issues/4032

Let's refactor queue processing out from the Cron class so it can be reused by other modules or Drush, without relying on cron itself.

CommentFileSizeAuthor
#10 3068764.10-queue-refactor.patch9.91 KBdeviantintegral
#6 3068764.5-queue-refactor.patch8.43 KBdeviantintegral
#2 3068764.1-queue-refactor.patch8.43 KBdeviantintegral

Comments

deviantintegral created an issue. See original summary.

deviantintegral’s picture

deviantintegral commented
Status: Active » Needs review
StatusFileSize
new 3068764.1-queue-refactor.patch8.43 KB

Here's a first pass. I'd like feedback before writing test coverage for the new class.

deviantintegral’s picture

deviantintegral commented
Related issues: +#1832818: Allow a queue item to be postponed
deviantintegral’s picture

deviantintegral commented
Issue tags: +Needs tests

Status: Needs review » Needs work

The last submitted patch, 2: 3068764.1-queue-refactor.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

deviantintegral’s picture

deviantintegral commented
Status: Needs work » Needs review
StatusFileSize
new 3068764.5-queue-refactor.patch8.43 KB

Fixes wrong brackets when claiming items.

neclimdul’s picture

neclimdul
He/Him
commented 
+++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,98 @@
+    while ((time() < time() + $process_time) && ($item = $queue->claimItem($lease_time))) {

(time() < time() + $process_time)

That doesn't look right...

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/Cron.php
@@ -98,15 +89,14 @@ class Cron implements CronInterface {
-  public function __construct(ModuleHandlerInterface $module_handler, LockBackendInterface $lock, QueueFactory $queue_factory, StateInterface $state, AccountSwitcherInterface $account_switcher, LoggerInterface $logger, QueueWorkerManagerInterface $queue_manager, TimeInterface $time = NULL) {
+  public function __construct(ModuleHandlerInterface $module_handler, LockBackendInterface $lock, QueueFactory $queue_factory, StateInterface $state, AccountSwitcherInterface $account_switcher, LoggerInterface $logger, QueueProcessor $queue_processor, TimeInterface $time = NULL) {

    it needs trick to provide BC and deprecate argument 

  2. +++ b/core/lib/Drupal/Core/Cron.php
@@ -162,46 +152,11 @@ protected function setCronLastTime() {
+   * @deprecated In Drupal 8.8.0, will be removed in Drupal 9.0.
    */
   protected function processQueues() {
...
+    $this->queueProcessor->processCronQueues();
   }

    Deprecation should be according to https://www.drupal.org/core/deprecation#how-param 

  3. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,98 @@
+    while ((time() < time() + $process_time) && ($item = $queue->claimItem($lease_time))) {

    no reason for 2 syscalls, there's time service 

  4. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,98 @@
+        watchdog_exception('cron', $e);
...
+        watchdog_exception('cron', $e);

    should be logger

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

deviantintegral’s picture

deviantintegral commented
Status: Needs work » Needs review
StatusFileSize
new 3068764.10-queue-refactor.patch9.91 KB

(time() < time() + $process_time)

no reason for 2 syscalls, there's time service

I originally had this since we need time updated on every iteration and thought it was easier to read. I've changed it to a local variable.

it needs trick to provide BC and deprecate argument

It's a service, so according to the BC docs we don't need to preserve compatibility?

Deprecation should be according to https://www.drupal.org/core/deprecation#how-param

Done! There's a placeholder for the change record node ID.

should be logger

Yes, but then we'd have to inline the contents of watchdog_exception into a private function. Since #2932518: Deprecate watchdog_exception is still needs work it seems ok to use the procedural function while it's not deprecated?

johnwebdev’s picture

johnwebdev commented
Status: Needs review » Needs work 
  1. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+  public function processQueue(string $queue_name, int $process_time, int $lease_time) {

    So not sure if we do scalar type hinting, yet? 

  2. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+        // release the item and skip to the next queue.
...
+        // Skip to the next queue.

    These comments should be removed. 

  3. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+        watchdog_exception('cron', $e);

    Should the type really be "cron" here? 

  4. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+  public function processCronQueues() {

    I'm wondering if we should remove this method, and keep the existing method in Cron, and simply update it to use this service. That would mean this service is decoupled from any runner.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Coming here from #3228000: Users deleted via JSON:API DELETE don't follow the site-wide cancel_method in the user settings where we really need queue processing for JSON:API

  1. +++ b/core/lib/Drupal/Core/Cron.php
@@ -98,15 +89,14 @@ class Cron implements CronInterface {
+  public function __construct(ModuleHandlerInterface $module_handler, LockBackendInterface $lock, QueueFactory $queue_factory, StateInterface $state, AccountSwitcherInterface $account_switcher, LoggerInterface $logger, QueueProcessor $queue_processor, TimeInterface $time = NULL) {

    we'll need to remove the typehint here for BC reasons, and check the type of the passed object.

    If it's a queue factory, we'll need to trigger a deprecation error and fetch the queue processor from the \Drupal singleton

    I know you've mentioned above that this isn't consistent with our docs, but when the effort is minimal, we provide BC for service constructors to make the minor release process less arduous. 

  2. +++ b/core/lib/Drupal/Core/Cron.php
@@ -162,46 +152,13 @@ protected function setCronLastTime() {
+   * @deprecated in Drupal 8.9.0 and is removed in Drupal 9.0.0.
...
+    @trigger_error('Cron::processQueues is deprecated in Drupal 8.9.0 and is removed in Drupal 9.0.0. Instead, use the queue_processor service. See https://www.drupal.org/node/NEEDS-CHANGE-RECORD', E_USER_DEPRECATED);

    these need updating now 

  3. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+  private $queueManager;
...
+  private $queueFactory;
...
+  private $time;

    We normally use protected in core to allow for an extension point 

  4. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+   * @param \Drupal\Component\Datetime\TimeInterface $time

    missing a comment 

  5. +++ b/core/lib/Drupal/Core/QueueProcessor.php
@@ -0,0 +1,111 @@
+  public function processQueue(string $queue_name, int $process_time, int $lease_time) {

    let's typehint this with a void return

  6. I'm wondering if we should remove this method, and keep the existing method in Cron, and simply update it to use this service. That would mean this service is decoupled from any runner.

    Yes, I agree that's a good idea

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#1189464: Add an 'instant' queue runner
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3242216: Support JSON:API asynchronic tasks
bradjones1’s picture

bradjones1
Primary language English
commented
Title: Refactor cron queue processing into it's own class » Refactor cron queue processing into its own class

Grammar fix in title.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mile23’s picture

mile23
Primary language English
Location Seattle, WA
commented

I suggest this should be more formally part of the Queue API (rather than cron), under Drupal\Core\Queue namespace and the service named queue.processor.

Also +1 on #11.4.

(Closed #3008997: Split \Drupal\Core\Cron::processQueues into its own class as a duplicate of this issue.)

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

moshe weitzman’s picture

moshe weitzman commented

Would still be helpful

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.