menu_router unreliable during rebuild
| Project: | Drupal |
| Version: | 6.12 |
| Component: | menu system |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | active |
Jump to:
First, feel free to correct if I am mistaken about my understanding of how menu_router and the rebuild process works.
I think this is related to #251792: Implement a locking framework for long operations and #246653: Duplicate entry in menu_router: Different symptoms, same issue in the sense that the menu_router_rebuild process should generally be locked to prevent two processes from rebuilding at the same time.
When menu_router gets rebuilt, it seems to enter into an unreliable state (after the DELETE from {menu_router} and before all of the router inserts are finished). If anyone loads the site during this time, there seems to be no mechanism to prevent them from querying a menu_router table that is not ready. I.e., if you ask for admin/blah/path/module, that particular router entry may not exist yet, so you may get the menu entry for admn/blah instead, or maybe nothing at all.
This is probably not a problem for a small blog, but for a high-traffic website, a rebuild window of even 1s is too big to not be noticed by someone.
The ideal situation would be that the READs from the table would be locked against WRITEs. I attempted wrapping those queries inside of SQL locks to see if that would help, and in practice this was way too slow -- seemed to just result in a lot of processes waiting for locks to clear whenever a rebuild happened.
Following a brief IRC discussion with Webchick and chx, an alternative was brought up where the menu_router table would be supplemented with a version/sequence/timestamp number, and then a system variable would be maintained to point requests to the latest "complete" set of menu router rows. In other words, you would never just wipe the table altogether -- instead, there would be some kind of cleanup process, so the menu_router table would contain a rolling set of menu_router entries.
Thoughts? I have this strategy working pretty well on a relatively high traffic site, but am always open to a better/simpler solution. Also note that this presents some difficulties with modules that currently directly query the menu_router table. E.g., not just menu.inc, but system.module, system.admin.inc, ctools, Ubercart, etc.
Note that I'm marking this as a bug for now because I think it would be expected that a URL should not have a failure rate no matter how unlikely the scenario (er, when the cause is known).
#1
Transactions seem to take care of this problem pretty well. A mere two lines of code added to menu.inc (before DELETE from menu_router: db query start transaction; after the inserts, db query commit), and I'm not able to trigger the incorrect behavior that I was seeing before without using the transaction.
This method also plays nice with other modules because no schema changes are necessary.
#2
Would you kindly write out those mere 2 lines of golden code that can save me from another day of misery? Thx.
#3
Sure, sorry, reading back I see my comment was kind of hard to read.
1. In menu.inc, look for the line that has "DELETE from menu_router". Right before that line, insert: db_query("start transaction");
2. The code that follows after that has some db_query("insert ... lines. After that block of code that handles inserting into menu_router, before the function ends, put in one more database call: db_query("commit");
Hope that helps.
#4
Pete S wrote: before the function ends, put in one more database call...
Please confirm. Should the line
db_query("commit");come just before or afterreturn $menu;?I tried it both ways and issue still not resolved. Here are the precises steps that I took (based on Drupal 6.13 menu.inc file):
1. Go to Line 2327:
// Delete the existing router since we have some data to replace it.db_query("start transaction");
db_query('DELETE FROM {menu_router}');
// Apply inheritance rules.
foreach ($menu as $path => $v) {
$item = &$menu[$path];
if (!$item['_tab']) {
// Non-tab items.
$item['tab_parent'] = '';
$item['tab_root'] = $path;
}
2. Add new line, just after the comment (before the
db_query('DELETE... line):db_query("start transaction");3. Find Line 2411 (now)
db_query("INSERT INTO {menu_router}4. Move to end of function. Just before Line 2433 (
return $menu;), add new line:db_query("commit");5. Upload revised menu.inc file and cross fingers.
#5
That's correct (putting the "commit" line after return $menu won't work because "return" terminates the function.) So, if you've followed those steps as you described, the problem should be resolved (assuming that this is in fact the only problem you're having.)
I would also double check the type and version of your database -- this is a MySQL-specific solution (although I believe any database would be affected by the problem. I believe that it should work in most recent versions of MySQL, but let me know if you are running MySQL 4.x or something.
Aside from that, if your site is still having trouble, you may need to troubleshoot beyond this particular issue.
#6
As noted, your suggestion did not fix my issue. And, of course, I have been troubleshooting it in many different ways:
WHAT ELSE I'VE TRIED:
1. Run update.php
2. Cleared Cached Data (admin/permissions)
3. Check and rebuild permissions
4. Simply loading the admin/modules page; also navigating thusly:
?q=admin/build/modules [instructions for rebuilding menus; any other way?]
5. menu_rebuild()
6. Checked memory limits settings (settings.php, .htaccess, php.ini). Set at
64mb.
a. Uninstall any unused modules [a later troubleshoot measure]
7. Restoring the subject tables, and even entire db, with working live site
sql dump.
8. Modified the menu.inc file as shown above.
9. Verified that all node IDs are valid.
#7
Well... you still haven't said what exact problem you're seeing, so I have no idea whether this thread is even relevant for you.