The menu_rebuild() function has code inside of it to ensure that during install and update, it will always be called again on the next page request. That seems a little crazy, and likely contributes to slowing down both install.php and update.php, as well as the first page load after a new Drupal installation.
CVS annotate says this was introduced in #202955: Access denied after install - menu_rebuild() calls to work around some problems people were seeing on a fresh installation. However, since now in Drupal 7 we already flush all caches (which includes a menu rebuild) right at the end of the installer, it is hard to see why we need to force one more rebuild on the next page.
If I just revert that code, I can experience at least one of the same bugs people reported in that issue (access denied on the settings page for Bartik, i.e. for the default theme). However, the root of that bug seems to be that the {menu_router} table is storing entire copies of each theme object in it, and those are out-of-date and have "status" set to 0, presumably because they resulted from a file system scan during the installer rather than obtained from the {system} table like they should be.
Drupal 7 can come to the rescue again here. There is no reason to store the entire theme object in the {menu_router} table, since Drupal 7 can check theme access based on the theme name alone. Storing the entire object seems like it could only lead to other bugs, where the menu system is working with an out-of-date copy of the theme.
If I fix both of those issues at the same time (as in the attached patch), I couldn't reproduce any problems after light testing of install.php and update.php. It needs more testing, though, including by the testbot.
In terms of backwards compatibility (now that we are in RC2), in theory if some contrib module stored entire theme objects in hook_menu() like block.module and system.module did, and if that module is included in an install profile, it could experience some permissions problems after installation as a result of this patch that would need a menu_rebuild() to fix. However, storing entire theme objects seems like a bug in and of itself, so the hypothetical contrib module should really fix itself anyway like I am trying to do for core here :)
Comment | File | Size | Author |
---|---|---|---|
#32 | menu-rebuild-too-often-997918-32.patch | 2.83 KB | achton |
Comments
Comment #1
catchLooks good on a first pass, subscribing.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks sound too. I'm eager to see if this increases testing speed.
Comment #3
effulgentsia CreditAttribution: effulgentsia commented+1 from me too. If we can make the maintenance page environment not leave stale data in the menu tables, that is better. I agree that storing full theme objects in {menu_router} is a bad idea, and neither core, nor contrib modules should do it, and if they do, then it's their responsibility to figure out how to trigger another menu rebuild when what they've stored becomes stale. Theme objects aside, is there anything else about the maintenance page environment that can leave stale {menu_router} data around?
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis leads to no test speed increase. I guess that Simpletest was not affected by those rebuilds.
Comment #5
David_Rothstein CreditAttribution: David_Rothstein commentedIn general, there shouldn't be much. It can only happen if hook_menu() uses an API function that specifically cares about the value of MAINTENANCE_MODE. I just looked through all of core's hook_menu() implementations and things looked relatively good (most don't even use any API functions at all) but there was one big exception: field_ui.module specifically checks MAINTENANCE_MODE in hook_menu().
That is fixed in the attached patch. As far as I can tell, it was also a case of crufty code left lying around that no longer served any purpose.
However, this suggests we might want to be pretty careful with this patch for Drupal 7.
Another possible issue that could arise is that since drupal_flush_all_caches() does not call menu_rebuild() at the very end, but rather does a bunch of cache_clear_all() calls after it, the installer could be basing its last menu_rebuild() off of incorrect cached data, and the fact that we currently rebuild the menu on the first page load after installation works around that. I don't know of a specific case where that happens, but it could. It seems like it would make more sense for drupal_flush_all_caches() to call menu_rebuild() at the end rather than in the middle, perhaps.
Comment #6
Sheldon Rampton CreditAttribution: Sheldon Rampton commented+1 from me too on this patch. The current code (on my D6 site) made maintenance mode very funky for me the other day when I needed to take the site offline for an hour to do update.php followed by some manual configuration changes. I tried dividing up the manual configuration work between myself and another developer, and the mere fact that two of us were simultaneously working on the site meant that our menu_rebuilds were bumping into each other and generating warning messages due to race conditions. It was a D6 site, not D7, and I understand that the menu_router race conditions have been fixed in D7, but even so, I think rebuilding the menu upon each page load is excessive.
Comment #7
catchThis passes tests, is cleaner, and should save a bit of memory, nothing not to like.
Comment #8
Dries CreditAttribution: Dries commentedCommitted to 8.x. Haven't committed it to 7.x yet although that looks like the right thing to do. Probably good for webchick to have another look at it.
Comment #9
David_Rothstein CreditAttribution: David_Rothstein commentedI'd be extremely wary of putting this in D7 at this point. Leaving at "needs review" for now.
Comment #10
bfroehle CreditAttribution: bfroehle commentedCould we at least commit the parts of this patch which relate to not storing the entire theme object in the menu tables?
Comment #11
catchI think we should commit this to D7, there are regular reports of installs timing out and going over the PHP memory limit, menu_rebuild() is a likely contributor.
Marking this back to RTBC, David if you have specific reasons why you think it's a bad idea to commit this, please explain.
Comment #12
yareckon CreditAttribution: yareckon commentedsub.
Comment #13
David_Rothstein CreditAttribution: David_Rothstein commentedWell, based on the above we know of a couple modules in core that needed to change their hook_menu() implementations to accommodate this, or else we would have had some nasty bugs introduced. So it's certainly possible there are contrib modules in the same boat? That's the main concern I would have.
It also seems like the switch from storing objects to strings would be a (tiny) API change, that could affect modules which implement hook_menu_alter() on these items. That one is probably less likely in practice.
Anyway, is this really even a bug? I know I'm the one who marked it as one originally, but I think I was stretching it :) It would be interesting to check if this cuts down on memory usage, but since there are still several menu rebuilds during the installer even with the patch, I'm not sure it will actually have a huge effect.
Comment #14
webchickAgreed that some benchmarks here would be nice, esp. given Damien's comment about the lack of simpletest speed-up. If we're going to potentially break more modules in contrib, let's make sure it's justified.
However, that change to the theme key seems like a straight-forward fix.
Comment #15
fagoI think the real problem is that menu_rebuild() doesn't work like a usual cache system, but rebuilds the menu immediately. Instead it should clear the built menu and rebuild the menu on-demand.
The current way of immediate rebuild leads to unnecessary menu_rebuilds(), as triggered by this issue. Consider configuration items having menu items: Those items need to trigger menu_rebuilds() when they are changed. Then, if a module adds some off this items during installation, it would trigger multiple menu_rebuilds although they menu would have been rebuild anyway (due to the module installation).
I just run into exactly that with profile2 types + the profile2 pages module. For now I add a workaround for it, though we might want look into improve menu_rebuild() to generally solve those issues by moving to a rebuild-on-demand strategy.
Comment #16
sunThe hook_menu() fixes in Block and System module should land as soon as possible.
Just stumbled over that theme registry info after doing an SQL diff of full database dumps for another issue.
Comment #17
slashrsm CreditAttribution: slashrsm commentedI think that I had problems on my site, that were caused by the excessive menu rebuilds. It looks like in D7 we call menu_rebuild() even in maintenance mode. Since I'm not so experienced in core, I do not know why is that, but is definitely a performance disadvantage.
It looks like this patch fixes this somehow, so I'd like to see this commited.
I agree with fago (#15), though. Maybe we could rethink the way menu rebuilding is done in D8.
Cross posting related issue: #1417930: Site goes down after revert of fields (to many database connections)
Comment #18
sunFor D7,
can we leave out the menu_rebuild() in MAINTENANCE_MODE and installer changes from #15?
And just fix the hook_menu() implementations to not put the entire theme object into the menu router?
Attached patch does just that.
Comment #19
mgifford#18: drupal.menu-theme-objects.18.patch queued for re-testing.
Comment #20
xjmComment #21
rbrandon CreditAttribution: rbrandon commentedComment #22
mgifford@rbrandon - what's different in the new patch?
Comment #23
rbrandon CreditAttribution: rbrandon commented@mgifford, sorry thought there was a comment there. That is patch for D7 that includes the "menu_rebuild() in MAINTENANCE_MODE and installer changes" since they are still valid changes and were left out of #18.
Comment #24
mgiffordThanks for the explanation.
Comment #29
joseph.olstadPatch #21 still applies however with offsets so I've rerolled the same.
Comment #30
joseph.olstadComment #32
achtonRerolled patch from #29 for Drupal 7.70. Part of the patch was commited in #1861604, so it no longer applied cleanly.