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 :)

CommentFileSizeAuthor
#32 menu-rebuild-too-often-997918-32.patch2.83 KBachton
#29 menu-rebuild-too-often-997918-29.patch3.45 KBjoseph.olstad
#21 menu-rebuild-too-often-997918-21.patch2.83 KBrbrandon
#18 drupal.menu-theme-objects.18.patch1.64 KBsun
#5 menu-rebuild-too-often-997918-5.patch4.13 KBDavid_Rothstein
menu-rebuild-too-often.patch3.33 KBDavid_Rothstein
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Looks good on a first pass, subscribing.

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

Looks sound too. I'm eager to see if this increases testing speed.

effulgentsia’s picture

effulgentsia CreditAttribution: effulgentsia commented 
+++ includes/menu.inc	14 Dec 2010 13:30:28 -0000
@@ -2544,10 +2544,7 @@ function menu_reset_static_cache() {
- * {menu_links}. If called from update.php or install.php, it will also
- * schedule a call to itself on the first real page load from
- * menu_execute_active_handler(), because the maintenance page environment
- * is different and leaves stale data in the menu tables.

+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?

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented

This leads to no test speed increase. I guess that Simpletest was not affected by those rebuilds.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
FileSize
menu-rebuild-too-often-997918-5.patch4.13 KB

Theme objects aside, is there anything else about the maintenance page environment that can leave stale {menu_router} data around?

In 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.

Sheldon Rampton’s picture

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.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

This passes tests, is cleaner, and should save a bit of memory, nothing not to like.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 8.x-dev » 7.x-dev

Committed 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.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented
Status: Reviewed & tested by the community » Needs review

I'd be extremely wary of putting this in D7 at this point. Leaving at "needs review" for now.

bfroehle’s picture

bfroehle CreditAttribution: bfroehle commented

Could we at least commit the parts of this patch which relate to not storing the entire theme object in the menu tables?

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

I 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.

yareckon’s picture

yareckon CreditAttribution: yareckon commented

sub.

David_Rothstein’s picture

David_Rothstein CreditAttribution: David_Rothstein commented

Well, 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.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Needs review

Agreed 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.

fago’s picture

fago
Primary language German
Location Vienna
CreditAttribution: fago commented

I 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.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

The 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.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented

I 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)

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
FileSize
drupal.menu-theme-objects.18.patch1.64 KB

For 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.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

#18: drupal.menu-theme-objects.18.patch queued for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +needs profiling
rbrandon’s picture

rbrandon CreditAttribution: rbrandon commented
FileSize
menu-rebuild-too-often-997918-21.patch2.83 KB
mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

@rbrandon - what's different in the new patch?

rbrandon’s picture

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.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford commented

Thanks for the explanation.

  • Dries committed 9ecd0e5 on 8.3.x 
    - Patch #997918 by David_Rothstein: menu gets rebuilt continually during...

  • Dries committed 9ecd0e5 on 8.3.x 
    - Patch #997918 by David_Rothstein: menu gets rebuilt continually during...

  • Dries committed 9ecd0e5 on 8.4.x 
    - Patch #997918 by David_Rothstein: menu gets rebuilt continually during...

  • Dries committed 9ecd0e5 on 8.4.x 
    - Patch #997918 by David_Rothstein: menu gets rebuilt continually during...
joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
FileSize
menu-rebuild-too-often-997918-29.patch3.45 KB

Patch #21 still applies however with offsets so I've rerolled the same.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented
Related issues: +#1978176: Build menu_tree without loading so many objects

  • Dries committed 9ecd0e5 on 9.1.x 
    - Patch #997918 by David_Rothstein: menu gets rebuilt continually during...
achton’s picture

achton
Location Denmark
CreditAttribution: achton at Reload commented
Related issues: +#1861604: Skip module_invoke/module_hook in calling hook_watchdog (excessive function_exist)
FileSize
menu-rebuild-too-often-997918-32.patch2.83 KB

Rerolled patch from #29 for Drupal 7.70. Part of the patch was commited in #1861604, so it no longer applied cleanly.