Posted by PaulMagrath on September 30, 2008 at 2:49pm
| Project: | Drupal core |
| Version: | 7.x-dev |
| Component: | node.module |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
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.
Comments
#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.
#11
Had the same problem as PaulMagrath, changing $context['sandbox']['current_node'] = $loaded_node->nid to $context['sandbox']['current_node'] = $row['nid']; worked well for me.
#12
Re-test of nodebug.patch from comment #8 was requested by moshe weitzman.
#13
Code looks good. More robust. Bot is happy.
#14
Interesting. So over at #471080: Trigger watchdog when node rebuild permissions fails, which is essentially the same fix, chx threatens to won't fix due to it only masking the issue.
The big question in my mind is whether or not this will still pick it up when a node does indeed fail to rebuild permissions, due to corrupt data or similar. If it does, then this is good to go.
Could someone test this to make sure (doing something like manually inserting an invalid uid into the node table's uid column will do it)? Even better, write automated tests for the rebuild permissions functionality to prove this, and/or point out where in core they already exist?
#15
#8: nodebug.patch queued for re-testing.
#16
I can understand chx's point of view. Fact is though that entries in the database are going to get corrupt sometimes either from hard drive failure, db admin's fiddling with database table contents directly or from people misusing the APIs. Drupal shouldn't fail like this when that happens. This is a small change for a significant increase in the robustness of the code of the function.
To answer the big question in your mind: there is no logging when a node fails to rebuild currently and this doesn't add it. That is being discussed currently over at #471080 I believe. This issue is for the fix itself only.
I've tested this fix to make sure it works and it has been tested and found to work by others as well. I agree that automated tests would be better and if you have the time, it'd be great if you get around to writing some!
In the meantime, I'm going to move this issue back to "reviewed & tested by the community". That reflects its current status and maturity. The lack of automated tests such as you describe is not directly relevant IMHO and shouldn't block this issue being fixed in HEAD.
#17
I think the problem lies in other part of this function, there's a race condition between:
$context['sandbox']['max'] = db_query('SELECT COUNT(DISTINCT nid) FROM {node}')->fetchField();and
$nids = db_query_range("SELECT nid FROM {node} WHERE nid > :nid ORDER BY nid ASC", 0, $limit, array(':nid' => $context['sandbox']['current_node']))->fetchCol();if at some point between context initialization and last iteration of second query node gets deleted or added, following code will result in an infinite loop:
if ($context['sandbox']['progress'] != $context['sandbox']['max']) {$context['finished'] = $context['sandbox']['progress'] / $context['sandbox']['max'];
}
#19
Takes into consideration that DrupalDefaultEntityController removes (returned) and fixes potential progress bar overflow.
#20
subscribing
#21
Related for Drupal 6 only (already fixed in D7+): #600836: Batch API never terminates if you set $context['finished'] > 1