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
We can only review patch files. Since there is no file marking as active.
#2
+1 this fix, I've encountered the same error and it was resolved by changing that one line.
#3
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'].
#4
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
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.
#6
The last submitted patch failed testing.
#7
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
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:
#9
#8 where is the different?
#10
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.