When I'm calling message_subscribe_send_message() with set parameters of:

$subscribe_options['use queue'] = TRUE;
$subscribe_options['uids'] = $subscribers;

Than queue enters into a loop.

CommentFileSizeAuthor
#12 prevent_loop_of_message-2303795-12.patch910 bytesklaasvw
#11 prevent_loop_of_message-2303795-11.patch605 bytestripper54
#1 no_que_and_uid_loop.patch463 bytesbsandor
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bsandor’s picture

bsandor CreditAttribution: bsandor commented
FileSize
no_que_and_uid_loop.patch463 bytes

My Understanding is that $subscribe_options['queue'] is for deciding if we are calling message_subscribe_send_message() from the queue worker.

If that is the case than this code goes into a loop as it is called from XXXX and even when it's calles from there it contains XXX so it will add another line into {queue}.

if ($use_queue) {
    // Add item to the queue.
    $task = array(
      'mid' => $message->mid,
      'entity_type' => $entity_type,
      'entity_id' => $id,
      'notify_options' => $notify_options,
      'subscribe_options' => $subscribe_options,
      'context' => $context,
      'uid' => $last_uid,
    );

    $task['subscribe_options']['last uid'] = $last_uid;

    // Create a new queue item, with the last user ID.
    $queue->createItem($task);
  }

So my idea is that this part needs another condition for testing if $subscribe_options['queue'] is set. So first line should be:

if ($use_queue && empty($subscribe_options['queue'])) {

I'm attaching a patch that solved this issue for me.
I used this pathch with this one: https://www.drupal.org/files/issues/dupenotifications-2299083-1_0.patch

dmsmidt’s picture

dmsmidt
he/him
Primary language Dutch
Location Amsterdam 🇳🇱🇪🇺
CreditAttribution: dmsmidt commented

There are two occurrences in the code in which a queue task is added.
The first time a check for $subscribe_options['queue'] is already implemented.
But the second time it isn't. Additionally, for me, it is not clear at all why this task adding code block is there the second time at all.
I guess this second block of code can be safely removed.

Or was the plan to have a fallback for when the task times out?

<?php
// Check we didn't timeout.
if ($use_queue && $subscribe_options['queue']['end time'] && time() < $subscribe_options['queue']['end time']) {
  continue 2;
}
?>

But then still some logic is missing.

tanius’s picture

tanius CreditAttribution: tanius commented

I can confirm that the two patches referred in #1 fixed the issue for me.

Context, also for others searching for the cause to their problem: We experienced this issue when sending messages to specific uids in queue mode, using a custom module which used the message_subscribe module for sending. When configuring message_subscribe to not use the queue for sending (as possible in admin/config/system/message-subscribe), the issue went away. Otherwise, we would have a stuck job in the message_subscribe queue, as shown by drush queue-list. It was stuck, because it would not go away after running the queue with drush queue-run message_subscribe, but would send the same e-mails infinitely on every queue run.

bsandor’s picture

bsandor CreditAttribution: bsandor commented
Status: Active » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: no_que_and_uid_loop.patch, failed testing.

Status: Needs work » Needs review

Amitaibu queued 1: no_que_and_uid_loop.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: no_que_and_uid_loop.patch, failed testing.

Status: Needs work » Needs review

cesarmiquel queued 1: no_que_and_uid_loop.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: no_que_and_uid_loop.patch, failed testing.

tripper54’s picture

tripper54 CreditAttribution: tripper54 commented

I have just encountered this issue, processing a queue with drush queue-run which has an array of uids populated by a custom module.

The issue with patch #1 is if the processing run doesn't get through your list of users: either the number of users is greater than the range (default 100), or the task times out before finishing with all the users.

In this case, another queue item should be created with an array of the users that the first run didn't get to.

So what if, after sending the message, we remove the user element from the $subscribe_options['uids'] array:

      message_notify_send_message($cloned_message, $options, $notifier_name);
      unset($subscribe_options['uids'][$uid]);

Note there is a 'last uid' data point in the queue item saved after the run, but that is never referenced.

tripper54’s picture

tripper54 CreditAttribution: tripper54 commented
Status: Needs work » Needs review
FileSize
prevent_loop_of_message-2303795-11.patch605 bytes

Patch attached using this method.

klaasvw’s picture

klaasvw CreditAttribution: klaasvw as a volunteer and at Randstad Digital commented
Version: 7.x-1.0-rc2 » 7.x-1.x-dev
Priority: Normal » Major
FileSize
prevent_loop_of_message-2303795-12.patch910 bytes

The above patch worked for me.

I refactored the code slightly:

  • Added an isset check to prevent warnings
  • Also added a check so no queue items are added when there are no further uids to process
oturpin’s picture

oturpin CreditAttribution: oturpin commented

Hi All,

I have the issue. But I am not sure that the intent of the developper was to reduce the $subscribe_options array whan message is sent. If I understand the code, the last_uid is stored and then used for next queue to proceed the uids over the last_uid up to range.
The code has a bug that make the queue starts all over again to index 0. Please, see that topic .

Waiting for your feedback !

oturpin’s picture

oturpin CreditAttribution: oturpin commented

Hi All,

@klaasvw : I like the approach. It simply works. I removed the last_uid logic from the code and from the queue. It runs OK.

Thx for your fix !

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Would it be possible to add a simpleTest to confirm the behavior of the queues?

bluegeek9’s picture

bluegeek9 CreditAttribution: bluegeek9 as a volunteer commented
Status: Needs review » Closed (outdated)