claimItem()'s docs say:

   *   On success we return an item object. If the queue is unable to claim an
   *   item it returns false. This implies a best effort to retrieve an item
   *   and either the queue is empty or there is some other non-recoverable
   *   problem.

However, it's possible to return NULL here:

    while (TRUE) {
      try {
        $item = $this->connection->queryRange('SELECT data, created, item_id FROM {' . static::TABLE_NAME . '} q WHERE expire = 0 AND name = :name ORDER BY created, item_id ASC', 0, 1, [':name' => $this->name])->fetchObject();
      }
      catch (\Exception $e) {
        $this->catchException($e);
        // If the table does not exist there are no items currently available to
        // claim.
        return FALSE;
      }
      if ($item) {
        *** get an item ***

        // Try to update the item. Only one thread can succeed in UPDATEing the
        // same row. We cannot rely on REQUEST_TIME because items might be
        // claimed by a single consumer which runs longer than 1 second. If we
        // continue to use REQUEST_TIME instead of the current time(), we steal
        // time from the lease, and will tend to reset items before the lease
        // should really expire.
        $update = $this->connection->update(static::TABLE_NAME)
          ->fields([
            'expire' => time() + $lease_time,
          ])
          ->condition('item_id', $item->item_id)
          ->condition('expire', 0);
        // If there are affected rows, this update succeeded.
        if ($update->execute()) {
          $item->data = unserialize($item->data);
          return $item;
        }

        *** fail to run the update query ***
      }
      else {
        // No items currently available to claim.
        return FALSE;
      }

     *** fall out of the if() block and end up here ***
     *** end of method returns NULL ***
    }

Therefore, there should be an else for the if ($update->execute()) block that returns FALSE.

CommentFileSizeAuthor
#11 update_databasequeue_claimitem_easy_read-3038506-11.patch2.4 KBvadim.hirbu
#9 update_databasequeue_claimitem_easy_read-3038506-9.patch2.24 KBvadim.hirbu
#4 retun-false-on-update-query-fail-3038506-2.patch522 bytesvadim.hirbu
#2 retun-false-on-update-query-fail-3038506-2.patch1.24 KBvadim.hirbu

Comments

joachim created an issue. See original summary.

vadim.hirbu’s picture

vadim.hirbu commented
StatusFileSize
new retun-false-on-update-query-fail-3038506-2.patch1.24 KB

Add return FALSE when update query is failing.

vadim.hirbu’s picture

vadim.hirbu commented
Status: Active » Needs review
vadim.hirbu’s picture

vadim.hirbu commented
StatusFileSize
new retun-false-on-update-query-fail-3038506-2.patch522 bytes
joachim’s picture

joachim commented
Status: Needs review » Reviewed & tested by the community

Thanks for the patch! Looks good.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Er... I'm not sure this is right. We're in a while (TRUE) { loop so if if ($update->execute()) { is not successful I think we're going to loop around again.

I think this is closed works as designed.

joachim’s picture

joachim commented

Yes, I think you're right.

I wonder if we should refactor this method to make it easier to see this is what's going on?

Maybe instead of:

      if ($item) {
      }
      else {
        // No items currently available to claim.
        return FALSE;
      }

have a guard clause that goes:

      if (!$item) {
        // No items currently available to claim.
        return FALSE;
      }

      // Actually do stuff if we're still here.
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: DatabaseQueue::claimItem() can sometimes return NULL, which is not part of its spec » Make DatabaseQueue::claimItem() easier to read
Category: Bug report » Task

Sure we can do that. Maybe something like

    while (TRUE) {
      try {
        $item = $this->connection->queryRange('SELECT data, created, item_id FROM {' . static::TABLE_NAME . '} q WHERE expire = 0 AND name = :name ORDER BY created, item_id ASC', 0, 1, [':name' => $this->name])->fetchObject();
      }
      catch (\Exception $e) {
        $this->catchException($e);
      }

      // If the table does not exist there are no items currently available to
      // claim.
      if (empty($item)) {
        return FALSE;
      }

      // Try to update the item. Only one thread can succeed in UPDATEing the
      // same row. We cannot rely on REQUEST_TIME because items might be
      // claimed by a single consumer which runs longer than 1 second. If we
      // continue to use REQUEST_TIME instead of the current time(), we steal
      // time from the lease, and will tend to reset items before the lease
      // should really expire.
      $update = $this->connection->update(static::TABLE_NAME)
        ->fields([
          'expire' => time() + $lease_time,
        ])
        ->condition('item_id', $item->item_id)
        ->condition('expire', 0);
      // If there are affected rows, this update succeeded.
      if ($update->execute()) {
        $item->data = unserialize($item->data);
        return $item;
      }
    }

Is easier to grok... less returns in a loop do make things simpler.

vadim.hirbu’s picture

vadim.hirbu commented
Status: Needs work » Needs review
StatusFileSize
new update_databasequeue_claimitem_easy_read-3038506-9.patch2.24 KB

Updated code according latest mentions and tested on latest version of drupal (8.8.x). All works as expected. Added patch.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work 
      catch (\Exception $e) {
        $this->catchException($e);
        // If the table does not exist there are no items currently available to
        // claim.
        return FALSE;
      }

can now become

      catch (\Exception $e) {
        $this->catchException($e);
      }

For one less return.

vadim.hirbu’s picture

vadim.hirbu commented
Status: Needs work » Needs review
StatusFileSize
new update_databasequeue_claimitem_easy_read-3038506-11.patch2.4 KB
johnhelmuth’s picture

johnhelmuth
he/him
commented
Issue tags: +Seattle2019
johnhelmuth’s picture

johnhelmuth
he/him
commented
Status: Needs review » Reviewed & tested by the community

Reviewed code, seems OK to me with the changes last requested. Patch applied cleanly. Site seems to work when running with this patch applied.

johnhelmuth’s picture

johnhelmuth
he/him
commented

Reviewed code and it looks OK to me, and includes the last changes requested.

Patch applied cleanly. Ran queue related tests on simplytest.me and they passed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: update_databasequeue_claimitem_easy_read-3038506-11.patch, failed testing. View results

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review

All looks good so setting back to Needs Review! Tests failure is not related (see #3055648: Frequent random fail in \Drupal\Tests\media_library\FunctionalJavascript\MediaLibraryTest).

subhojit777’s picture

subhojit777
he/him
Primary language Bengali
commented
Issue tags: +#DCD19
borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

This does make it a lot more readable, I'm a big fan of this change. Great work!

  • catch committed e5fc916 on 8.8.x 
    Issue #3038506 by vadim.hirbu: Make DatabaseQueue::claimItem() easier to...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed e5fc916 and pushed to 8.8.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.