Allow a queue item to be postponed

queue_postpone.patch queued for re-testing.

moveToTheEnd is consistent with the implementations given but is it the correct terminology for what a generic queue would do? it seems more in line with the reliable interface.

Very cool though and a useful feature.

Looks pretty solid and with tests. I like the explicit weight instead of the implicit weighting by date a lot. Lets bump it up.

I agree with RTBC but note that this depends on #1837118: UPDATE foo SET bar=(SELECT...) is not supported, which has been updated in the meantime. The easiest way might be to RTBC and commit the other issue and then re-roll this one.

Fair point, the old version of that fix is included. per IRC discussion postponing this but the relevant code is RTBC by everyone involved pending the commit of the listed fix.

woot, back in action. needs reroll

reroll here

Patch in #13 is identical to the one in #8 minus the already commited UpdateQuery related change. Back to RTBC.

I don't understand why we have to do all these tricky queries instead of just SELECT then UPDATE, and the code comments don't explain why either (they explain why we can't do other tricky queries but that's not the point). If the issue is reliability then why isn't a transaction and SELECT .. FOR UPDATE enough?

if this gets re-rolled again, can you please add a type hint to '@param $item' in the interface class? Thanks.

Other minor documentation issue is that there needs to be a '@throws' directive in any method docblock that throws an exception.

I think of moveToTheEnd as reEnqueue. Then you can go implement a "LIFO/Stack" queue without confusion.

#23: 1832818_23.patch queued for re-testing.

This is my first reroll. Because of this I wasn't comfortable changing moveToTheEnd to reEnqueue or even adding any documentation. FWIW, I agree that moveToTheEnd should be renamed to something like reEnqueue.

Fixed syntax error within previous patch.

#30: 1832818-allow-queue-item-postponement_28.patch queued for re-testing.

Renamed moveToTheEnd() to reEnqueue().

Thanks @colinafoley! I found a few minor things in the patch that we can clean up.

1. +++ b/core/lib/Drupal/Core/Queue/DatabaseQueue.phpundefined
   @@ -138,4 +138,20 @@ function __construct($name, Connection $connection) {
   +   * Implements Drupal\Core\Queue\QueueInterface::reEnqueue().
   
   +++ b/core/lib/Drupal/Core/Queue/Memory.phpundefined
   @@ -108,4 +108,12 @@ class Memory implements QueueInterface {
   +   * Implements Drupal\Core\Queue\QueueInterface::reEnqueue().
   

   Nowadays we can replace these with {@inheritdoc}.

2. +++ b/core/lib/Drupal/Core/Queue/QueueInterface.phpundefined
   @@ -109,4 +109,12 @@
   +   * Move an item to the end of the queue.
   

   Super minor nitpick: This should be "Moves" rather than "Move".

3. +++ b/core/lib/Drupal/Core/Queue/QueueInterface.phpundefined
   @@ -109,4 +109,12 @@
   +   * @param $item
   +   *   The item returned by Drupal\Core\Queue\QueueInterface::claimItem().
   ...
   +  public function reEnqueue($item);
   

   We need a data type for $item here (and, if it's an object or an array, an appropriate typehint in the method and its implementations).

Tagging novice for those cleanups.

Also, we'll need some test coverage, so tagging for that.

reEnqueue doesn't make sense if we don't have en enQueue method. We should use reCreateItem unless we want to rename the other method.

We also need better documentation for this method. I look at it and I wonder why we're making an API addition to save the runner two commands. I'm guessing it's for transactional reasons, which we should make clear if so.

Updating the patch with issues noted in #34
I also found a few other places {@inheritdoc} seemed appropriate so I have adjusted those as well as another doc issue I found.

Wow, sorry about the attach/multi post mess - not used to DO D7 yet =(

assigned to myself

Hi this is my first patch ever. I'm new to Drupal (discovered it 2 days ago!) and PHP in general (discovered last week! :D)
Attached is a re-roll of the patch from comment 38 with one change (see interdiff.txt)

42: 1832818-allow-queue-item-postponement_42_all.patch queued for re-testing.

Integrated feedback from @msonnabaum and changed reEnqueue to reCreateItem and added comment to describe why this was added in the interface.
Also inserted a \stdClass before ($item).

This is great work @Shaika, your patch addresses the concerns that @msonnabaum had at #35, and adds an API addition that is sorely missed in the queueing system.

The docblock tidy ups are also great to see, and I can confirm there are no leftover "Implements" in the concrete classes.

All tests pass, happy to RTBC this (would be good to see this backported to D7 if committed to D8).

I like it. Welcome to the community @shaika and thanks for the work on this, you nailed it!

Talked to mark and I think we came to a understanding of what he was asking for and it was a little bit more technical which isn't very novice. At the same time, I had some questions about why we _needed_ this and if it was just a special construct of abstracting our database implementation. With a little research it seems some bigger implementations don't explicitly support this but there might be ways to handle like returning failure in some cases. So I think mark may still be thinking about it but I'm happy with this.

Hey @neclimdul, thanks for taking the time to talk to @msonnabaum, the new comment you added looks like a positive step forward.

I have a +1 for this to be RTBC, I think it would be best if @msonnabaum gave it the RTBC ;)

Re-marking as RTBC as no-one seems to have any issues with this, removing tag "needs tests" as this patch has tests, also adding tag "api addition" as this is only and API addition (not a change).

Here is a reroll bringing this up to 8.7.x. The biggest difference is that the update hook only updates the indexes if the queue table exists. I'm unclear on why we need to change the indexes at all. I'm wondering if it's stale from when this was a moveToEnd implementation?

This patch actually uses the new reCreateItem() method when processing the cron queue. I can split it off into a new issue, but it ended up being so simple that I think it's OK to include here.

I've updated the issue summary as the prior notes were stale given this issue hadn't been fixed for them.

The test failures were caused by the update hook (which I hadn't run yet), and given that it's not clear the index changes are needed I've rolled them back.

I also have questions about the interface changes(addition of type hint). As they seem minimally connected to the patch and I'm pretty sure not BC with existing implementations for most of our supported php versions, they aren't allowed. I think this was targeting even pre-beta requirements so after 5 years this probably needs a really solid review to make sure it really fits what's expected to get into core.

> I also have questions about the interface changes(addition of type hint)

I was thinking the same thing. It should be fine for PHP 5.5, but I'd be much happier to roll those back and get this issue finished. I'll do that as I fix the current test fails.

• Logs when items are recreated by cron, so site owners can know if an item is stuck for a long period of time.
• Removes the \stdClass typehints.
• Adds RecreateException and a corresponding test class I missed adding to the prior patch.

Removes a spurious newline.

One issue (that I've had to work with every time in site code but forgot in the above patches) is handling when a short queue has failing items in it. #68 will repeatedly try to process the item, even though odds are high it will still fail. This causes an effective while true loop on both the queue to recreate the item, and any database or remote API calls.

This patch fixes that by keeping track of the IDs of items that have been recreated, and if they have been seen in the current cron run leaving them locked. As well, this fixes some missing parameter type docs and returns the newly created item ID in recreateItem.

I wonder if we should rename reCreateItem to postpone. Recreate makes sense for the database queues, but a queue in SQS might want to set a visibility timeout instead. And in those cases, it would make sense for queue workers to indicate how long that unavailable time should be, but I'm hesitant to add that as a method to an exception.

Anyways, let's see if this patch passes.

After discussing with a colleague, I decided to go ahead and rename recreate to postpone. Of course, it's the title of the issue so I think it's clear postponed is a term that will be widely understood.

I believe I briefly had a discussion with chx about this when he was posting the patch and the actual reason for re-queue is that the method doesn't provide any actual promise of postponing(delaying the run) of a item. It moves it to the end of the queue but with a queue depth of 0 you'll likely end up with the same loop on the item. This doesn't mean its less useful, only that postpone promises a behavior we're not documenting where "requeue" explicitly does.

The way postpone is implemented in #70, doesn't that make throwing PostponeItemException and any other Exception (that isn't RequeueException or SuspendQueueException) functionally equivalent?

At least for Database and Memory queues. They both will set a non-zero expiry value in their claimItem method. That expiry will be left alone in the case of any other exception from the queue worker (https://git.drupalcode.org/project/drupal/blob/8.8.x/core/lib/Drupal/Cor...). That effectively excludes them from being picked up again until the expiry passes (reset to 0 by system_cron). Meaning that if a queue worker wishes to postpone, it can just throw a custom exception and ensure that a reasonable value for cron.time is set in it's annotation. With the caveat that drush currently doesn't have the final catch that core cron has, so if your running drush queue:run it won't work. But that's a bug (https://github.com/drush-ops/drush/issues/4032) and if you want to use drush queue:run with this patch, you'd need Drush patched to support the same PostponeItemException.

I have a scenario where a postponed queue item would be ideal, but not the in the way this issue describes it.

For example, we have a system which sends text messages to users, but we don't want these notifications to be sent during sleeping hours, even if the action which triggers the notification happens during those hours.

I wish there was a way for the Queue API to simply ignore queue items until a specific date in the future. These would not be processed until the postponed date.

Is there any consideration for something like this? If not, I suppose I could look at the [Advanced Queue](https://www.drupal.org/project/advancedqueue) module, which seems to have a delayed processing feature.

I think this is something you'd want to handle before outside of the queue system. For example, while SQS has support for delayed sending, it's only a maximum of 15 minutes: https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGui...

If your case doesn't involve time zones, you perhaps you could suspend processing of the whole queue (but again, different backends may not support that).

Ah yes, I forgot to mention why I don't want to throw a SuspendQueueException. When you throw a SuspendQueueException, it logs an error in the database log. If I have cron running every minute, it would quickly write lots of noisy logs for what is basically not an error. :(

Re-rolling patch against 8.9

Hello. Thanks for the patch! I also need this.

I had just one problem with patch #78. When I throw PostponeItemException the item is expired in queue. So when I run cron again the item is not processed anymore.

The problem is produced by the $queue->claimItem() behaviour:

        while (time() < $end && ($item = $queue->claimItem($lease_time))) {
          // If this item has been postponed during this cron run, then keep it
          // locked and continue on to the next item. This ensures that in the
          // case of a queue with a single failing item that we don't spam the
          // queue or any upstream services, giving transient errors a chance to
          // fix themselves.
          if (in_array($item->item_id, $postponed_ids)) {
            continue;
          }

Even we postpone this item in the bucle, as we have claimed it, the item is already expired and it won't be processed again.

The solution to the problem is postponing the item, not when the exception is captured, but after all the queue items have been claimed. Attaching a patch that does it.

Please review, Thanks!

Another patch to fix tests. It was being check that the next item is false after postpone exceptions, but now it will contain an item, that will be processed in the next cron execution.

Hasn't this been fixed here? https://www.drupal.org/project/drupal/issues/3116478

Not super sure if it addresses the same use case, so could anyone confirm?

This is a duplicate of #3116478: Add a way to silently keep an item locked when processing a queue via cron from my perspective. The committed implementation is also better in perf as doesn't store items in memory.

removed