Simplify the default queue implementation

This came out of an argument between chx and me about the necessity of the sequences API. :-)

Tagging.

Needs to remove remaining mentions of consumer_id.

Looks good, tests pass, committed, moving on!

        // 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 = db_update('queue')
          ->fields(array(
            '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;
        }

This doesn't work, and lead to an infinite loop, slowing down the test infrastructure big time.

Oh... that's because we are selecting an item that is already claimed:

-      $item = db_query_range('SELECT data, item_id FROM {queue} q WHERE consumer_id = 0 AND name = :name ORDER BY created ASC', 0, 1, array(':name' => $this->name))->fetchObject();
+      $item = db_query_range('SELECT data, item_id FROM {queue} q WHERE name = :name ORDER BY created ASC', 0, 1, array(':name' => $this->name))->fetchObject();

Maybe we are missing a expire = 0 condition here?

[edit] or add a ->condition('expire', $item->expire) to the update query above.

It's not infinite; old items do get cleaned up. It's just woefully non-optimal. We do need an "expire = 0" condition in the initial range query for any hope at efficiency.

Well, not even woefully non-optimal. Successfully run tasks delete their entries after running. This just breaks concurrent lock acquisition.

It leads to an infinite loop on the server: the SELECT picks up an item with a expire <> 0, and the UPDATE tries to set the expire time of an item with expire = 0. That makes no sense.

Quick fix to the "expire" issue (which is probably indicative of problems elsewhere).

Hop, let's save the test bot, please.

Just a quick note on why this went into an infinite loop:

The test for the queue attempts to fetch two items before "processing" and deleting either. After acquiring a lock on the first item, it attempts to fetch a second. The existing code in HEAD then tried to acquire a new lock on the existing, locked item. This was impossible because the existing lock wouldn't be released until later in the same thread. The code also loops whenever it finds and item but can't acquire the lock.

Incidentally, this bug affected two types of operations. It effectively serialized any attempt at multithreaded operation. More importantly, it caused an infinite loop whenever a single thread attempted to acquire more than one item before processing and deleting any. It's not clear that the second situation occurs anywhere in core except the tests, but it is a possibly optimal use case under conditions where it's more efficient to process multiple items at once in one thread than go through serialized acquire-process-delete cycles.

Committed to CVS HEAD. Thanks!

This is still broken. I just checked out clean HEAD again this morning, cleared my DB, reinstalled D7, and my site can't fetch available update data at all. If I revert this patch, everything is fine. Can we please not "simplify" working APIs in the name of obscure performance gains? ;)

If this patch stays, class SystemQueue still has a protected $consumerId; that needs to go, too...

Btw, just tried again, and it's completely reproducible. Fresh install, default profile, visit admin/reports/available. Watch infinite loop. Revert this patch, try again, see your available updates report.

I have *NO* idea why the update tests are passing, but they are. :( That's a separate issue. ;)

I really don't have time today to debug this since I'm on a rampage to get #538660: Move update manager upgrade process into new authorize.php file (and make it actually work) finished. For now, I'm just going to leave this patch reverted in my workspace so I can finish plugin manager...

Ugh, sorry for the false alarm. Too much hacking, not enough sleep. ;) I just discovered that I had left #601600: First view of updates report without cached release data reports bogus results for projects running HEAD applied to my copy of cvs_deploy, which is what was causing the loop, not a failure to drain the queue as it first appeared. Something unholy was going on as I reverted this patch which is what made me think this was the culprit. Apologies.

Anyway, still needs work for:

If this patch stays, class SystemQueue still has a protected $consumerId; that needs to go, too...

but totally minor and can be done post 10/15...

This patch removes the stray $consumerId that dww pointed out in #17.

Committed to CVS HEAD. Thanks!