Node Access Rebuild never finishes (infinite loop)

PaulMagrath - September 30, 2008 - 14:49
Project:Drupal
Version:7.x-dev
Component:node.module
Category:bug report
Priority:normal
Assigned:Unassigned
Status:needs review
Description

Small bug in the core "node" module's node.module

Current code:

<?php
function _node_access_rebuild_batch_operation(&$context) {
....
  while (
$row = db_fetch_array($result)) {
   
$loaded_node = node_load($row['nid'], NULL, TRUE);
   
// To preserve database integrity, only aquire grants if the node
    // loads successfully.
   
if (!empty($loaded_node)) {
     
node_access_acquire_grants($loaded_node);
    }
   
$context['sandbox']['progress']++;
   
$context['sandbox']['current_node'] = $loaded_node->nid;
  }
...
}
?>

The last line in this while loop should read:
<?php
$context
['sandbox']['current_node'] = $row['nid'];
?>

As if the loaded is empty, $loaded_node->nid will be empty too causing an infinite loop in the batch operation, as it takes an empty value for $context['sandbox']['current_node'] as meaning it has not yet started the rebuild and starts all over again.

#1

earnie - September 30, 2008 - 23:02
Status:needs review» active

We can only review patch files. Since there is no file marking as active.

#2

btopro - October 20, 2008 - 18:01

+1 this fix, I've encountered the same error and it was resolved by changing that one line.

#3

coltrane - May 1, 2009 - 20:07
Status:active» needs review

I believe this fixes a problem that almost has had me pulling my hair out. I've rebuilt permissions a hundred times before and never had it stall or consistently keep saying 'The content access permissions have not been properly rebuilt.' Disabling and uninstalling modules hasn't fixed it on the site suffering from this problem so I dove into the batch process. It's easy to see that if not all nodes are processed in _node_access_rebuild_batch_operation() than the batch process reports it as unfinished and node_access_needs_rebuild(FALSE) is never called. What's difficult for me to deduce at this moment is why the node_load() may not return a full node object. However, since it's just wrong to use $loaded_node->nid in the case it might not be an actual node object we really should be using the $row['nid'].

AttachmentSizeStatusTest resultOperations
node-315302-3.patch675 bytesIdleFailed: Failed to apply patch.View details | Re-test

#4

coltrane - May 1, 2009 - 20:49

This is likely a problem in HEAD as well http://api.drupal.org/api/function/_node_access_rebuild_batch_operation/7 because if $node is empty then $context['sandbox']['current_node'] won't record the processed node id.

Edit: Spoke with Dave Reid on irc about this and node_load_multiple() doesn't return any invalid nodes so this bug probably won't occur in 7.

#5

yched - May 1, 2009 - 22:38
Version:6.x-dev» 7.x-dev

The fix makes complete sense, nice catch. I guess such cases, where there is a record in the node table but node_load() fails, can happen with deleted node types.

D7 can be affected too, though : if *no* valid node is found within the nid range, then $context['sandbox']['current_node'] won't be updated, and infinite loop ensues.

So, patch in #3 is RTBC for D6, but this will need to be fixed in D7 first. Here's a patch, needs review.

AttachmentSizeStatusTest resultOperations
fix_n_a_rebuild_batch-315302-5.patch1.53 KBIdleFailed: Failed to apply patch.View details | Re-test

#6

System Message - May 24, 2009 - 18:55
Status:needs review» needs work

The last submitted patch failed testing.

#7

martinquested - October 28, 2009 - 18:55
Status:needs work» needs review

This bug has just started to affect my D6 site. I can't see why the patch shouldn't work, the #3 patch works on my D6 site, and I can't see any errors in the automated testing results for #5. Can someone who knows more than me (that doesn't narrow it down much) review this manually and see if it's RTBC?

Thanks.

#8

PaulMagrath - November 14, 2009 - 15:15

Patch is failing because in Drupal 7 they have changed the for loop into a for each loop:

  $nids = db_query_range("SELECT nid FROM {node} WHERE nid > :nid ORDER BY nid ASC", 0, $limit, array(':nid' => $context['sandbox']['current_node']))->fetchCol();
  $nodes = node_load_multiple($nids, array(), TRUE);
  foreach ($nodes as $node) {
    // To preserve database integrity, only acquire grants if the node
    // loads successfully.
    if (!empty($node)) {
      node_access_acquire_grants($node);
    }
    $context['sandbox']['progress']++;
    $context['sandbox']['current_node'] = $node->nid;
  }

This should fix it:

  $nids = db_query_range("SELECT nid FROM {node} WHERE nid > :nid ORDER BY nid ASC", 0, $limit, array(':nid' => $context['sandbox']['current_node']))->fetchCol();
  $nodes = node_load_multiple($nids, array(), TRUE);
  foreach ($nodes as $nid => $node) {
    // To preserve database integrity, only acquire grants if the node
    // loads successfully.
    if (!empty($node)) {
      node_access_acquire_grants($node);
    }
    $context['sandbox']['progress']++;
    $context['sandbox']['current_node'] = $nid;
  }

I've attached a patch file for testing:

AttachmentSizeStatusTest resultOperations
nodebug.patch1.07 KBIdlePassed on all environments.View details | Re-test

#9

tobiasb - November 14, 2009 - 18:28

#8 where is the different?

#10

PaulMagrath - November 21, 2009 - 11:38

tobiasb:

The difference is that instead of getting the nid from the loaded node, you use the nid from the array. The nid from the array will always be valid whereas the nid from the loaded node will be undefined if the loading of the node fails for any reason.

 
 

Drupal is a registered trademark of Dries Buytaert.