Add a way to silently keep an item locked when processing a queue via cron

Nice. Can't find any need for improvement/nitpick and appears to work as intended. Sending to RTBC.

This is fantastic, thanks!

+++ b/core/tests/Drupal/Tests/Core/CronTest.php
@@ -0,0 +1,197 @@
+      $module_handler       = $this->createMock('Drupal\Core\Extension\ModuleHandlerInterface');
+      $lock_backend         = $this->createMock('Drupal\Core\Lock\LockBackendInterface');
+      $queue_factory        = $this->createMock('Drupal\Core\Queue\QueueFactory');
+      $state                = $this->createMock('Drupal\Core\State\StateInterface');
+      $account_switcher     = $this->createMock('Drupal\Core\Session\AccountSwitcherInterface');
+      $logger               = $this->createMock('Psr\Log\LoggerInterface');
+      $queue_worker_manager = $this->createMock('Drupal\Core\Queue\QueueWorkerManagerInterface');
+      $time                 = $this->createMock('Drupal\Component\Datetime\TimeInterface');
...
+        ->with('Exception')

Not sure if this kind of indentation is encouraged

Found the corresponding coding standard is Drupal.Formatting.MultipleStatementAlignment.NotSame, #2937853: Fix 'Drupal.Formatting.MultipleStatementAlignment' coding standard

I'd set back to NW, besides of this, RTBC +1

Should be Drupal.WhiteSpace.OperatorSpacing.NoSpaceBefore probably. But phpcs says that it does not exist

Thanks @clayfreeman

+++ b/core/tests/Drupal/Tests/Core/CronTest.php
@@ -0,0 +1,197 @@
+      $module_handler = $this->createMock('Drupal\Core\Extension\ModuleHandlerInterface');
+      $lock_backend = $this->createMock('Drupal\Core\Lock\LockBackendInterface');
+      $queue_factory = $this->createMock('Drupal\Core\Queue\QueueFactory');
+      $state = $this->createMock('Drupal\Core\State\StateInterface');

Just FYI, personally, I prefer using prophecy. See https://www.drupal.org/docs/8/phpunit/using-prophecy

This is a feature request and so I think it can only go into 9.1.x now.

Do we also need a kernel test, ie. extend or duplicate Drupal\Tests\system\Kernel\System\CronQueueTest?

+++ b/core/tests/Drupal/Tests/Core/CronTest.php
@@ -0,0 +1,197 @@
+      $this->assertNotEquals(TRUE, \Drupal::state()->get('cron_test.watchdog_exception_called'));
...
+      $this->assertEquals(TRUE, \Drupal::state()->get('cron_test.watchdog_exception_called'));

Is it neater to use assertFalse/assertTrue over assertEquals here?

I guess it is a minor addition with no disruption so this is perhaps OK for 8.9.x after all - let's leave that to the core committers to decide.

The changes look great, thanks for adding to CronQueueTest to prove that the delayed item works end to end, so marking this RTBC.

So I started thinking through if this gave other queue systems the tools to support this behavior and just catching an exception and continuing starts to seem kinda weird and hacky. You have no idea what the delay is going to be and its just what ever the claiming code sent as the lease for running the queue and now we're going to delay out the rest of the time. :( Shouldn't we be sending a delay? Which leads to should there be some sort of delayItem($item, $delay) method that delay's the item allowing say beanstalk to send an explicit delay call?

Maybe I'm wrong, the code is very simple, but I'm not sure this is the right approach.

Thank you, @clayfreeman!

1. +++ b/core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestException.php
   @@ -22,9 +23,13 @@ public function processItem($data) {
   -    else {
   +    elseif ($state->get('cron_queue_test_exception') === 1) {
          $state->set('cron_queue_test_exception', 2);
   ...
   +    elseif ($state->get('cron_queue_test_exception') === 2) {
   +      $state->set('cron_queue_test_exception', 3);
   

   Prefer replacing else-if with if to both two usages, The logic do not change if to do so.

2. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,197 @@
   +      $cron = new \ReflectionClass(Cron::class);
   +      $processQueues = $cron->getMethod('processQueues');
   +      $processQueues->setAccessible(TRUE);
   ...
   +      $processQueues->invoke($this->cron);
   ...
   +      $processQueues->invoke($this->cron);
   ...
   +      $processQueues->invoke($this->cron);
   ...
   +      $processQueues->invoke($this->cron);
   ...
   +      $processQueues->invoke($this->cron);
   

   It's tricky here, Cron::processQueues() is a protected method. Repalace with $this->cron->run(); the test should pass as well. $this->cron->run() does call Cron::processQueues()

3. +++ b/core/lib/Drupal/Core/Queue/DelayedRequeueException.php
   @@ -0,0 +1,60 @@
   +   * The interval of time that the item should remain locked (in seconds).
   ...
   +   * If this value is positive, the queue should discard the current lock expiry
   +   * time and update it by adding the specified interval to the current time.
   ...
   +   * @var int|float
   ...
   +  protected $delay = 0;
   

   Do not understand why to allow float. delay 0.5 sec? Make a little sense, if so, why not just use microseconds.

4. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,197 @@
   +    protected function setUp() {
   +      parent::setUp();
   ...
   +      $this->cron = new Cron($module_handler, $lock_backend, $queue_factory, $state, $account_switcher, $logger, $queue_worker_manager, $time);
   

   The ::setup() is about 70 lines long. not sure if using prophecy can make it better.

5. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,197 @@
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertTrue(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertFalse(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   ...
   +      $this->assertTrue(\Drupal::state()->get('cron_test.watchdog_exception_called'));
   

   \Drupal::state() are called many times, Set it a property in ::setup()?

6. +++ b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
   @@ -78,6 +78,24 @@ public function testExceptions() {
   +    $this->assertEqual(\Drupal::state()->get('cron_queue_test_exception'), 3);
   ...
   +    // The item should be left in the queue with delayed release.
   +    $this->assertEqual($queue->numberOfItems(), 1, 'Item is still in the queue.');
   

   Better to use $this->assertEquals()

7. Is a change record necessary? To tell developers how to use this.

Thanks for your quick response. @clayfreeman

1. #22.2. I tried simply replacing them, do not need to touch the setup(). see the interdiff. The decision is yours. I do not have a good reason for both. :)
2. #22.3 make sense.
3. #22.4 👍
4. #22.7 👍
5. I'd suggest allowing to set delay in QueueWorker annotation. For example
   

   /**
    * 
    *
    * @QueueWorker(
    *   id = "foo_worker",
    *   title = @Translation("Foo worker"),
    *   cron = {"time" = 60},
    *   delay = {"time" = 0}
    * )
    */
   

   Default to 0 -- no delay. So it won't break existed queue workers.

1. +++ b/core/lib/Drupal/Core/Queue/DelayableQueueInterface.php
   @@ -0,0 +1,32 @@
   +  public function releaseItem($item, $delay = 0);
   +
   

   Yeah, I actually agree with your BC note in context of this method signature because this change could be a problem. I didn't notice that's what you put in slack.

   The BC concern is why I suggested something like public function delayItem($item, $delay) in my earlier review because it is an entirely new method.. Its also a bit clearer, and I'm realizing now possibly allow you to _add_ an item with a delay which is handy.

2. +++ b/core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestException.php
   @@ -22,9 +25,13 @@ public function processItem($data) {
   -    else {
   +    if ($state->get('cron_queue_test_exception') === 1) {
          $state->set('cron_queue_test_exception', 2);
        }
   +    if ($state->get('cron_queue_test_exception') === 2) {
   +      $state->set('cron_queue_test_exception', 3);
   +      throw new DelayedRequeueException();
   +    }
   

   Totally a nit not relevant to the issue but these magic numbers are kinda hard to follow. Maybe we could make them constants on the test to tie everything together and self document what the states are?

   Also, this doesn't test the delay time. :(

3. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,182 @@
   +      $queue_worker_plugin = $this->prophesize('Drupal\Core\Queue\QueueWorkerInterface');
   +      $queue_worker_plugin->processItem('Complete')->willReturn();
   +      $queue_worker_plugin->processItem('DelayedRequeueException')->willThrow(DelayedRequeueException::class);
   +      $queue_worker_plugin->processItem('Exception')->willThrow(\Exception::class);
   +      $queue_worker_plugin->processItem('SuspendQueueException')->willThrow(SuspendQueueException::class);
   +      $queue_worker_plugin->processItem('RequeueException')->will(function ($args, $mock, $method) {
   

   oh this is a lot easier to read, nice work! I'm also a big fan of prophecy but I didn't want to say anything because a test is a test and I'm not writing it. :-D

4. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,182 @@
   +  if (!function_exists('watchdog_exception')) {
   +
   +    /**
   +     * Fake watchdog_exception() function used to throw an exception.
   +     */
   +    function watchdog_exception($type, \Exception $exception, ...$args) {
   +      \Drupal::state()->set('cron_test.watchdog_exception_called', TRUE);
   +    }
   +
   +  }
   

   This made me uncomfortable the first time I read it and I wasn't sure why but now thinking about it I realize the problem. It is sort of a ticking time bomb because if another test defines the method tests might pass or fail depending on the order the tests run in. I don't have a suggestion of the top of my head but yeah.

   I guess a real fix might be blocked on #2932518: Deprecate watchdog_exception which is no reason to hold this up. Can you just add @runInSeparateProcess to the test, that might avoid the problem for now. :(

@clayfreeman, #27 Looks good to me 👍🏻, Moving it to RTBC, Thanks.

Hi @Neslee Canil Pinto. From what I got from the conversations on slack between @neclimdul and @clayfreeman, @neclimdul will check back soon, who is the maintainer of Queue/QueueAPI. His opinions are important. So let's set it back to Needs Review.

Thanks everyone, I've got a few observations that should probably be noted for committers and i'm sorry, a nit that will make this easier to use.

1. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
   @@ -171,6 +171,31 @@ public function releaseItem($item) {
   +      $expire = time() + intval($delay);
   +      if ($expire < 0) {
   +        $expire = 0;
   +      }
   

   Observation, boy howdy you have to work hard to get a negative value to trigger this :) Probably could have just tested the delay since using a negative delay to set a time in the past doesn't really do anything I don't think but its all good.

2. +++ b/core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestException.php
   @@ -13,17 +16,36 @@
   +      throw (new DelayedRequeueException())->setDelay($delay);
   

   nit, could we just take the delay as a constructor argument to make this more straight forward?

3. +++ b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
   @@ -91,21 +119,21 @@ public function testExceptions() {
   -    $this->assertEqual($queue->numberOfItems(), 2, 'Failing queue stopped processing at the failing item.');
   +    $this->assertEquals(2, $queue->numberOfItems(), 'Failing queue stopped processing at the failing item.');
   ...
   -    $this->assertEqual($item->data, 'crash', 'Failing item remains in the queue.');
   +    $this->assertEquals('crash', $item->data, 'Failing item remains in the queue.');
        $item = $queue->claimItem();
   -    $this->assertEqual($item->data, 'ignored', 'Item beyond the failing item remains in the queue.');
   +    $this->assertEquals('ignored', $item->data, 'Item beyond the failing item remains in the queue.');
   

   good catch. not related but we'll keep it a secret between you me and all the people that read this comment ;)

hm... thought i'd kicked this up. I think this is looking very good, lets see what committers think.

I think a CR is necessary.

Here's some thoughts about the runtime code. I've not reviewed the tests yet.

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -180,6 +182,14 @@ protected function processQueues() {
   +            // The worker requested the task not be immediately requeued.
   +            if ($queue instanceof DelayableQueueInterface) {
   +              // This queue can handle a custom delay; use the duration provided
   +              // by the exception.
   +              $queue->delayItem($item, $e->getDelay());
   +            }
   

   What happens if $queue is not a DelayableQueueInterface and is that tested?

2. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
   @@ -171,6 +171,31 @@ public function releaseItem($item) {
   +      $expire = time() + intval($delay);
   

   Why the call to intval() - now we have PHP 7.3 we can typehint to integer.

3. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
   @@ -171,6 +171,31 @@ public function releaseItem($item) {
   +      if ($expire < 0) {
   +        $expire = 0;
   +      }
   

   I ponder if you mean here that $delay should always be positive. Imo we should throw an exception if delay is negative.

4. +++ b/core/lib/Drupal/Core/Queue/DelayedRequeueException.php
   @@ -0,0 +1,63 @@
   +  public function __construct($delay = 0) {
   

   Let's typehint $delay to integer.

5. +++ b/core/lib/Drupal/Core/Queue/DelayedRequeueException.php
   @@ -0,0 +1,63 @@
   +  /**
   +   * Set the desired delay interval for this item.
   +   *
   +   * @param int|float $delay
   +   *   The desired delay interval for this item.
   +   *
   +   * @see self::$delay
   +   *   For a value description of the $delay parameter.
   +   *
   +   * @return static
   +   *   The object itself for chaining.
   +   */
   +  public function setDelay($delay) {
   +    if (is_numeric($delay) && $delay >= 0) {
   +      $this->delay = $delay;
   +    }
   

   Let's not have a setter that gives this exception state that's unnecessary. Do the setting in the constructor.

The new functionality definitely looks useful.

Thank you @clayfreeman! All points in #37 are addressed.

+++ b/core/lib/Drupal/Core/Cron.php
@@ -180,6 +182,20 @@ protected function processQueues() {
+          catch (DelayedRequeueException $e) {
+            // The worker requested the task not be immediately requeued.
+            //
+            // If the queue doesn't support ::delayItem(), we should leave the
+            // item's current expiry time alone.
+            //
+            // If the queue does support ::delayItem(), we should allow the
+            // queue to update the item's expiry using the requested delay.
+            if ($queue instanceof DelayableQueueInterface) {
+              // This queue can handle a custom delay; use the duration provided
+              // by the exception.
+              $queue->delayItem($item, $e->getDelay());
+            }

But the comment to address #37.1, I would suggest changing it to the following:

          catch (DelayedRequeueException $e) {
            // The worker requested the task not be immediately re-queued.
            // - If the queue doesn't support ::delayItem(), we should leave the
            // item's current expiry time alone.
            // - If the queue does support ::delayItem(), we should allow the
            // queue to update the item's expiry using the requested delay.
            if ($queue instanceof DelayableQueueInterface) {

1) Use - to organize them in bullets, 2) requeued, should be re-queued probably, see #3138768: [Meta] Fix flagged spelling errors due to missing hyphens for prefixes

Otherwise, this is RTBC.

Updated patch #40 along with the changes suggested in #41, please review.

Thanks, @mrinalini9!

When did we start adding scalar typehints? Is this a new policy for D9.1?

@Charlie ChX Negyesi For now only a few test methods require type hints. See [Meta] Implement strict typing in existing code and Overridden test methods require void return type hints. Since Drupal 9 requires PHP 7.3 we can make it better.

1. +++ b/core/lib/Drupal/Core/Cron.php
   @@ -180,6 +182,18 @@ protected function processQueues() {
   +            // The worker requested the task not be immediately re-queued.
   +            // - If the queue doesn't support ::delayItem(), we should leave the
   +            // item's current expiry time alone.
   +            // - If the queue does support ::delayItem(), we should allow the
   +            // queue to update the item's expiry using the requested delay.
   +            if ($queue instanceof DelayableQueueInterface) {
   +              // This queue can handle a custom delay; use the duration provided
   +              // by the exception.
   +              $queue->delayItem($item, $e->getDelay());
   +            }
   

   Should there be some kind of message logged when the queue doesn't support ::delayItem()?

2. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
   @@ -171,6 +171,33 @@ public function releaseItem($item) {
   +    try {
   +      // Add the delay relative to the current time.
   +      $expire = time() + $delay;
   +      // Update the expiry time of this item.
   

   This should use the time service.

About #47.2, maybe we could add a time-service related Trait which is similar to the \Drupal\Core\Messenger\MessengerTrait trait to bypass the BC concerns, For example, call $this->getTime() to get the "datetime.time" service instance.

1. #47.1 @clayfreeman trended to not address it and explained in #48.1

2. +++ b/core/lib/Drupal/Core/Datetime/TimeTrait.php
   @@ -0,0 +1,43 @@
   +trait TimeTrait {
   ...
   +  public function time() {
   +    if (!isset($this->time)) {
   +      $this->time = \Drupal::time();
   +    }
   +
   +    return $this->time;
   +  }
   +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
   @@ -11,9 +12,10 @@
   +  use TimeTrait;
   
   @@ -171,6 +173,33 @@ public function releaseItem($item) {
   +      $expire = $this->time()->getCurrentTime() + $delay;
   

   #47.2 was addressed via introduced the TimeTrait which wrapped the\Drupal::time(); service and suggested by me in #49. I am not sure if it is a good idea. But if it's not, I would suggest moving forward here, furthermore, rescoping #3123216: Replace non-test usages of \Drupal::time() with IoC injection and refactoring it with IoC in #3123216, removing the TimeTrait there if necessary.

So setting back to RTBC, to have opinions from committer(s) again.

Thanks!

I discussed this issue with @catch. Never of us are keen on adding a TimeTrait in this issue. I think the best way forward here is use \Drupal::time() here and then open a follow-up to change _batch_queue to use the class resolver service so queue classes can get their dependencies injected.

Whilst looking at the above I also realised that one impact of this change is that now core's batches via \Drupal\Core\Queue\Batch will automatically support this functionality. I'm not sure whether this is an intended side-effect - or even desired - I've not thought it through yet. But, if Batch does support this then I think we need to make sure that that is documented properly.

1. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.php
   @@ -89,7 +89,7 @@ protected function doCreateItem($data) {
   -        'created' => time(),
   +        'created' => \Drupal::time()->getCurrentTime(),
   
   @@ -140,7 +140,7 @@ public function claimItem($lease_time = 30) {
   -          'expire' => time() + $lease_time,
   +          'expire' => \Drupal::time()->getCurrentTime() + $lease_time,
   
   @@ -171,6 +171,33 @@ public function releaseItem($item) {
   +      $expire = \Drupal::time()->getCurrentTime() + $delay;
   
   +++ b/core/lib/Drupal/Core/Queue/Memory.php
   @@ -12,6 +12,7 @@
    class Memory implements QueueInterface {
   +
   
   @@ -44,7 +45,7 @@ public function createItem($data) {
   -    $item->created = time();
   +    $item->created = \Drupal::time()->getCurrentTime();
   
   @@ -63,7 +64,7 @@ public function numberOfItems() {
   -        $item->expire = time() + $lease_time;
   +        $item->expire = \Drupal::time()->getCurrentTime() + $lease_time;
   

   this changes looks out of scope but could use separate issue to inject time service otoh all this places are valid to use time() as exact system time is needed. Better to add new child into #2729597: [meta] Replace \Drupal with injected services where appropriate in core

   The only reason to do injection of new service is better ability to unit-test the code, but not sure it makes sense

2. +++ b/core/lib/Drupal/Core/Queue/DelayableQueueInterface.php
   @@ -0,0 +1,34 @@
   +   * @param mixed $item
   +   *   The item returned by \Drupal\Core\Queue\QueueInterface::claimItem().
   

   instead of "mixed" better use 'object'

3. +++ b/core/lib/Drupal/Core/Queue/DelayedRequeueException.php
   @@ -0,0 +1,53 @@
   +  public function __construct(int $delay = 0) {
   +    if ($delay >= 0) {
   +      $this->delay = $delay;
   

   I think set value makes sense only when $delay > 0 (as default is 0 in class property)

4. +++ b/core/lib/Drupal/Core/Queue/DelayedRequeueException.php
   @@ -0,0 +1,53 @@
   +  public function getDelay() {
   

   needs type-hint to int

#55 one impact of this change is that now core's batches via \Drupal\Core\Queue\Batch will automatically support this functionality. I'm not sure whether this is an intended side-effect - or even desired - I've not thought it through yet. But, if Batch does support this then I think we need to make sure that that is documented properly.

#56 I'm not familiar with the batch subsystem, so I'll need further guidance on what to do to make sure the documentation updates are squared away.

Batch operations are documented in core/includes/form.inc. Also core/lib/Drupal/Core/Form/form.api.php callback_batch_operation() may be a relevant place to document. Neither place currently mention anything about how exeptions (including the existing RequeueException) are handled.

But the first task is to establish what effect throwing DelayedRequeueException in a batch operation currently has with this patch. Does the item get processed again at the end of the batch? Does it get left in the queue for the next time the batch is run? Does it get orphaned in the queue and will never run? Does it get silently discarded? It's possible this needs a test.

\Drupal\Core\Queue\Batch::claimItem() disregards item expiry entirely in its query. \Drupal\Core\Queue\BatchMemory isn't susceptible to any repercussions since it doesn't implement DelayableQueueInterface.

Makes sense.

Seems to me the feature is RTBC, but the wonderfully extensive test coverage could use a few tweaks.

1. +++ b/core/modules/system/tests/modules/cron_queue_test/src/Plugin/QueueWorker/CronQueueTestException.php
   @@ -13,17 +16,36 @@
   +      $state->set('cron_queue_test_exception', self::GENERIC);
   ...
   +      $state->set('cron_queue_test_exception', self::NONE);
   +    }
   ...
   +      $state->set('cron_queue_test_exception', self::DELAYED);
   

   Updating the state from within the test queue plugin seems odd. I understand it was how it was done before, but now that this test is getting more complicated maybe it would be best to move these into testExceptions() to make the flow of the test logic more explicit.

2. +++ b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
   @@ -41,6 +70,75 @@ protected function setUp(): void {
   +  public function testTimeService() {
   +    $this->assertEquals($this->currentTime, \Drupal::time()->getCurrentTime());
   +    $this->assertEquals($this->currentTime, \Drupal::time()->getRequestTime());
   +  }
   

   Probably better to do these sanity tests within setup rather than a a seperate test.

3. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,195 @@
   +   * This test must run in a separate process so that our custom
   +   * `watchdog_exception()` doesn't pollute global scope for other tests.
   ...
   +    /**
   +     * Fake watchdog_exception() function used to throw an exception.
   +     */
   +    function watchdog_exception($type, \Exception $exception, ...$args) {
   

   watchdog_exception is scheduled for deprecation in #2932518: Deprecate watchdog_exception. What we usually do is mock/prophesize the logger. See e.g. core\tests\Drupal\Tests\Core\Block\BlockManagerTest

4. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,195 @@
   +    public function testProcessQueues() {
   

   Feels like this could use a dataProvider to be more DRY

1. I find The difference between the memory queue and database queue test plugins, and how they're tested and asserted, hard to wrap my head around. But it is well commented and it does serve a purpose, so I think it's OK. I can't seem how to simplify it without losing some angle of test coverage

2. +++ b/core/modules/system/tests/src/Kernel/System/CronQueueTest.php
   @@ -41,6 +61,77 @@ protected function setUp(): void {
   +  protected function tearDown(): void {
   +    $this->container->set('queue', $this->realQueueFactory);
   +  }
   

   I'm surprised if this is needed. Drupal's test isolation is usually rather good as is.

3. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,170 @@
   +    // Create a mock logger to set a flag in the resulting state.
   +    $logger = $this->prophesize('Drupal\Core\Logger\LoggerChannelInterface');
   +    $logger->log(Argument::cetera())->will(function () {
   +      \Drupal::state()->set('cron_test.message_logged', TRUE);
   +    });
   

   The obvious question here is why we're not using $logger->expects() and are doing this complicated state dance instead. At the very least this needs a comment explaining. I suspect the reason is that we need to know the arguments from the dataProvider to set the logger expectation and we don't know these in setUp(). But why not move the logger setup into the test method itself? Seems cleaner to me.

4. +++ b/core/tests/Drupal/Tests/Core/CronTest.php
   @@ -0,0 +1,170 @@
   +      if ($method_calls === 1) {
   +        throw new DelayedRequeueException();
   +      }
   

   It would seem conceptually simpler to have this item return NULL on the second call, the same as the 'Complete' item? Because RequeueException is what we care about here, not DelayedRequeueException.

Awesome! I had to implement this feature in Drupal 7 from scratch for processing a huge amount data via a 3rd party system that could have failed anytime, it was years ago but this feature finally gets supported in Drupal 8 9! RTBC++

This also works for me

The test coverage is a bit tricky here, but I don't have ideas to simplify it, and the coverage it adds is good.

We need to add 'delayable' to the cspell dictionary, but otherwise RTBC for me.

CSPELL: checking all files
..core/lib/Drupal/Core/Cron.php:11:23 - Unknown word (Delayable)

Also we should open an issue against drush to support this once it's added here - since that's the main queue runner in contrib.

Added "delayable" into core/misc/cspell/dictionary.txt

$ yarn spellcheck:core
yarn run v1.22.4
$ cspell "**/*" "../composer/**/*" "../composer.json"
CSpell: Files checked: 14725, Issues found: 0 in 0 files
✨  Done in 249.56s.

Also we should open an issue against drush to support this once it's added here - since that's the main queue runner in contrib.

Tagging "Needs followup" for this.

Setting back to RTBC.

Thanks!

Oh, my bad, @clayfreeman, thanks!

Committed 0f10d21 and pushed to 9.1.x. Thanks!

Opened the follow-up for drush: https://github.com/drush-ops/drush/issues/4543

@clayfreeman, congrats on your first Drupal core contribution!

Created #3177922: DelayedRequeueException should call parent, and optionally allow providing default args to cover improving \Drupal\Core\Queue\DelayedRequeueException.

Here is a re-roll for 9.0.x with #3177922: DelayedRequeueException should call parent, and optionally allow providing default args included.

#3183220: Add support for pre-delayed queue items

The follow-up on this issue: allow mutating the queue item's data between processing attempts.

Folks may be interested in #3198868: Add delay to queue suspend too

I don't think this is related to implementing the coding standards in core.