#1484216: Race condition in _update_create_fetch_task() (PDO Exceptions) exposed some one-off code in update module that attempts to keep queue items unique. Maintaining a queue alongside a separate key/value store, itself based on a forked version of the cache system, is not ideal.

Seems like having queue class for unique items could be useful generally, and would definitely simplify that part of update module.

CommentFileSizeAuthor
#19 really_D7_this_time.patch2.52 KBcatch
#17 update_fetch_tasks_D7.patch2.51 KBcatch
#10 do_not_delete_fetch_tasks-with-tests.patch2.55 KBBerdir
#10 do_not_delete_fetch_tasks-only-tests.patch1.44 KBBerdir
#9 do_not_delete_fetch_tasks.patch1.11 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented

redis_ssi module bends over backwards to implement this. Huge, huge +1

Ps. You are welcome to borrow the code from there ;)

MrMaksimize’s picture

MrMaksimize CreditAttribution: MrMaksimize commented

We've been working on a module for some of our own needs in Drupal 7 centered around synchronizing feeds and REST calls. I think this is a good start to a unique queue system http://drupal.org/sandbox/MrMaksimize/1399810 and with some molding could be adapted to core.

At the moment, the module adds two tables to the queue table - q_key and q_item_id. Since it's feeds adapted, we're using q_key to store feed_nid, and q_item_id to store the serial id of the data we're importing (user, node, etc). At the moment it's pretty much adapted only for systemQueue, I haven't even tried with the memoryQueue

My coworkers pointed me to this issue and we'd love to help out with getting this into core!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Priority: Normal » Major

Since this is the 'proper' fix for #1484216: Race condition in _update_create_fetch_task() (PDO Exceptions) I'm going to bump it to major.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

i don't like the idea of a 'unique queue'. i'd prefer we keep the 'unique' tracking out of the queue and in a tracking table.

that's sort of what the code does now, only really badly. if we stop treating that like a cache table, and run the update task creation through a single function that checks for existing queue entries via a tracking table, we're done.

if we put this idea in the queue, we can no longer swap it out. how, for example, do you implement a 'unique queue' with beanstalkd?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

As per discussion in IRC, this could be a separate API.

In fact, isn't it very similar to locking, just without timeout?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

completely untested code:


function drupal_unique_queue_item_create(QueueInterface $queue, $data, $key) {
  $status = db_merge('unique_queue_item')
    ->key(array('key' => $key))
    ->fields(array(
      'key' => $key,
      'created' => REQUEST_TIME, 
    ))  
    ->execute();
  if ($status === Drupal\Core\Database\Merge::STATUS_INSERT)
    $queue->createItem($data);
  } 
}

then _update_create_fetch_task becomes:

function _update_create_fetch_task($project) {
  $key = 'fetch_task::' . $project['name'];
  drupal_unique_queue_item_create(queue('update_fetch_tasks'), $key, $project);
}

UPDATE: fix stupid typo with unique item function call.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

less derp-filled version, so it's possible to know how old a queue item is:

function drupal_unique_queue_item_create(QueueInterface $queue, $data, $key) {
  $status = db_merge('unique_queue_item')
    ->key(array('key' => $key))
    ->insertFields(array(
      'created' => REQUEST_TIME,
    ))
    ->updateFields(array(
      'updated' => REQUEST_TIME,
    ))
    ->execute();
  if ($status === Drupal\Core\Database\Merge::STATUS_INSERT) {
    $queue->createItem($data);
  }
}
Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

One issue with this is that the module that is using this function needs to additionally place $key somehow in in $data itself, so that it can then call drupal_unique_queue_item_delete($key). Or know how to regenerate the key based on $data.

Wondering if it would make sense to use the name of the queue as well in key(), that would help to prevent conflicts between different queues and possibly allow to clear a queue if need be. Does make it even harder to do the delete as you need to load the queue again there, which means that you can't have a worker callback that can handle multiple queues or you need to add that information into $data as well :(

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
FileSize
do_not_delete_fetch_tasks.patch1.11 KB

I've been thinking about this a bit.

Backporting this to Drupal 7 might not be easy.

However, from what I can see, fixing the actual bug in update.module is absolutely trivial.

All we need to do is ensure that calls to _update_cache_clear() without argument do not clear the fetch task thingies. That's almost a one line change, see attached patch. Because update.module already makes sure that it clears the specific fetch task when he is processed. Additionally, it is possible that calls without a specific cid should actually only delete a bunch of specific cid's.. for example, there is no need to delete fetch data for 200 projects if you enable a theme. In fact, doing that is madness ;) (And no, it's not sparta either).

Not sure if we want to do this here, in the existing issue or create a new one. I think either the other one or a new task and then this one is not a major task but a normal and maybe even a feature request that does not need to be backported.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Active » Needs review
FileSize
do_not_delete_fetch_tasks-only-tests.patch1.44 KB
do_not_delete_fetch_tasks-with-tests.patch2.55 KB

Ok, attached are tests for this bug, with and without the fix.

Status: Needs review » Needs work

The last submitted patch, do_not_delete_fetch_tasks-only-tests.patch, failed testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review

Uh, since when does the testbot set an issue to needs work if one of the patches does not pass tests?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

@berdir, depends on the order of the patch files. Also more or less forever:)

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Add a queue for unique items » Update module creates duplicate queue items
Category: task » bug
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

#10 looks good to me, it'd be good to abstract this out, but let's get the duplicate items issue fixed.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Component: base system » update.module

Changing component.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

just a me too, this is the simplest possible fix, and is ready to fly IMO.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 8.x-dev » 7.x-dev
FileSize
update_fetch_tasks_D7.patch2.51 KB

Committed/pushed to 8.x. Patch applies cleanly to 7.x with p2, so attaching that and leaving at RTBC for 7.x.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, update_fetch_tasks_D7.patch, failed testing.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs work » Needs review
FileSize
really_D7_this_time.patch2.52 KB

queue() vs. DrupalQueue::get() in the test.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

nice fix catch, this is ready to go.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Opened a new issue for the original purpose of this one. #1548286: API for handling unique queue items

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to D6
jonhattan’s picture

jonhattan
Primary language Spanish
Location Plasencia
CreditAttribution: jonhattan commented
Issue tags: -Needs backport to D7

Removing tag

Status: Patch (to be ported) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Version: 6.x-dev » 7.x-dev
Issue summary: View changes
Status: Closed (outdated) » Fixed

Never got backported to D6, but seems more appropriate to call this "fixed"...

Status: Fixed » Closed (fixed)

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