Easy to trigger multiple menu rebuilds per page (including infinite recursion) via menu_get_item() since 7.12

I'm having exactly the same memory problem. Version 7.5 works fine, but as soon as I update to 7.7 the site stops working. Before it worked perfectly with 128M memory limit and 30 seconds timeout, in 7.7 it even fails with 512M memory and 60 seconds. There must be an endless loop...

The interesting thing is, that I've updated another site to 7.7 without any problems...

I disagree that the code within menu_rebuild() is guilty for the loop because I've compared this function with the one of v 7.5 and it's exactly the same. The check for MAINTENANCE_MODE isn't new.

But maybe you're on the right way because when I compared the menu.inc of 7.5 to the one of 7.7, I saw that the call of menu_rebuild() has been moved to another place: in 7.5 it was called from within menu_execute_active_handler() (line 496). In 7.7 this call was moved to the function menu_get_item() (line 437)

As I've already tried to update and afterwards rolled back the site several times today, I don't have the nerves at the moment to check, if everything works, if I change this code snippet back to its old place. Because currently is 7.5 up and running.. and if I'd update now again and this wasn't the cause for the error, I would simply throw my computer out of window and scream... So I won't try this now, but I hope I could give you guys a good hint for solving the problem...

Many thanks in advance!!!

Have a nice weekend!
Andy

PS: I've upgraded the issue priority to "critical" because it completely stops Drupal to work if it's concerned by this issue. So this is definitely a blocker!

Cannot reproduce either. Could someone with an affected site traceback where the calls to menu_get_item() come from?

My affected site will go public today or tomorrow. In the mean time editors are working on the content heavily, so I can't do tests against this installation. Neither I have time to set up a test environment today, but as soon as the site goes online, I can do this

Great work! Big thanks! I do use the custom_search module in the affected site...

At first sight, the call to menu_get_item() in custom_search_theme() doesn't look important to me, so I could strip it out and go straight to update to 7.7, but the question is, if that call is really so "illegal". imho the system shouldn't break down so easily, if a simple call of the l() or menu_get_item() function is done at the wrong places (another question is: is it really the wrong place?? I personally don't think so). Drupal is such a solid system and I hope that this behavior will be changed in order to be more bulletproof...

I thought I'd seen other modules doing this, and I think what I was remembering is devel, which actually calls url() instead of l(), which seems to avoid the problem entirely. I doubt anyone actually needs to use l() in hook_menu(), so suggesting use of url() within hook_menu() in the documentation may be enough. I assume all of the function calls in that loop are there for a reason, so it's maybe not worth re-architecting around an easily avoidable problem.

Ok, I will open up a bug report within the custom search module, asking them to avoid the menu_get_item() call. (thanks mjpa, you were quicker again :) ) For the same purpose they use it now, a simple database query would be far more efficient anyway:

    $search_results_page_callback = menu_get_item('search/node/%menu_tail');
    if ($search_results_page_callback['page_callback'] == 'page_manager_search_page') {
      unset($custom_search_theme_array['search_results'], $custom_search_theme_array['search_result']);
    }

This really isn't a core bug this is bad code in each of the contrib modules. Drupal doesn't baby sit broken code as a rule.

marcingy, please make more effort to follow our community code of conduct.

If the code isn't changing, the documentation should. There's currently zero indication of how contrib authors are supposed to do links in hook_menu(). Or are they not supposed to do it at all? This is just going to come up again and again if it doesn't get documented somewhere, while contrib authors repeatedly waste their time trying to track down the same problem. That's not a suitable resolution.

Please don't quote things at me like the code of conduct after all my decision is as valid as yours and just because you don't like an issue being closed as won't fix doesn't mean it is the wrong decision. I'm sorry but in my opinion this is not a core bug I could do many things that would cause drupal core to break and we simply can't document them all and nor should we. And if you want documentation then I suggest you be pro-active and create a patch :)

Oh and we fix things in the latest release first anyway

Adding backport tag

I would be happy to submit a patch if we have more consensus that updating documentation is the best way to resolve this. So far, that's only my personal leaning while everyone else has expressed a preference for code changes.

I didn't intend to suggest at all that closing the issue is a code of conduct problem. To be clearer, I believe referring to contrib volunteers as babies is disrespectful. If that wasn't your intent, and I hope it wasn't, then I suggest choosing your words a little more carefully. I do not believe it is inappropriate to reference the code of conduct in situations like this. That's what it's there for.

No where do I refer to contrib maintainers as babies I simply stated that the problem is bad code in the modules in question so don't twist what I said. And it is a well quoted fact that drupal does not baby sit broken code in the same way as we are never backwards compatable between versions.

I am a contrib module volunteer myself so I deal with the idiocracies of drupal quite often so most certainly am not disrespectful to contrib maintainers at all because I am one. My point could maybe have been phrased better maybe but the basic point I was trying to make is that a module can do a lot of things that cause problems and those problems might end up being common but they aren't fault of core after all someone may simple choose to ignore the docs that have been created anyway and there is no way we can cover anything a module can do to break core.

An obvious place to start dealing with this issue on the hook_menu page on api.drupal.org and put a comment of don't use l() in the description index of a menu item because it may break sites and instead use url().

Good idea. Done.

Is everyone else okay with dropping the code changes and just updating the documentation?

I've added another comment in the documentation.

So, if you intend to use l() while in hook_menu(), definitely try the code snippet I've posted.

Hi, I'm the maintainer of the deadly (in this case) Custom Search module.
The faulty code comes originally from a patch, to play better with Page Manager (Chaos Tools), in D6.
When upgrading the module to D7, everything seemed fine… until 7.7.
Following advices (not to use get_menu_item), I've just replaced

$search_results_page_callback = menu_get_item('search/node/%menu_tail');
if ($search_results_page_callback['page_callback'] == 'page_manager_search_page') {
  unset($custom_search_theme_array['search_results'], $custom_search_theme_array['search_result']);
}

by

$router_item = db_query_range('SELECT page_callback FROM {menu_router} WHERE path = :path', 0, 1, array(':path' => 'search/node/%'))->fetchAssoc();
if ($router_item['page_callback'] == 'page_manager_search_page') {
  unset($custom_search_theme_array['search_results'], $custom_search_theme_array['search_result']);
}

It seems to be working for now.
Do you think it's ok? A better idea someone?

A baby maintainer ;-)

Retitling this to focus more on the root cause, which is basically that menu_get_item() can now trigger a menu rebuild in response to the 'menu_rebuild_needed' variable. This was introduced in #898634: install_drupal(): call to install_display_output() results in SQL syntax error.

I came across a slight variation of the same problem on a production Drupal 7 site. There:

1. The site had the 'menu_rebuild_needed' variable set to TRUE, so the first call to menu_get_item() would trigger the rebuild.
2. Unfortunately, due to some other bug (probably in a contrib module) the rebuild failed due to a duplicate key error in {menu_router}. This means the menu rebuild got rolled back, so the 'menu_rebuild_needed' variable was still there.
3. The next call to menu_get_item() therefore triggered another attempt, and the same thing happened over again. Something like 30 times per page request, thereby causing an insane slowdown of the site (and an enormous amount of memory being used).

Obviously the root cause there was the duplicate key error, but still, I think we're seeing another example of this being more fragile than it needs to be.

Looking at the patch that was committed in #898634: install_drupal(): call to install_display_output() results in SQL syntax error, I wonder if we can meet both goals by moving the 'menu_rebuild_needed' check back to menu_execute_active_handler() where it was, but leaving the 'menu_masks' check inside menu_get_item()...?

Here is a patch implementing my suggestion above. It is completely untested; that includes both manual and automated tests... It probably needs both before we would commit it :)

But for now let's think about whether it's the correct approach at all.

subscribing

I've been running this patch on a development site for the past 4 weeks or so and it definitely fixes the issue I ran in to and has not seemed to cause any other issues. So it's no longer "completely untested" as there has been some manual black-box testing :)

Same issue with me - recursion in menu_rebuild().
I re-roll the patch to be more drush make friendly (no core in the path).
It will fail unit tests but it works with drush make

BTW, are the core devs aware that this is not only a 8.x issue, but affects *all* stable D7 releases starting with 7.12?

#22: menu-get-item-rebuild-1232346-22.patch queued for re-testing.

#22: menu-get-item-rebuild-1232346-22.patch queued for re-testing.

This is an important issue, but I don't believe it's a major bug (see http://drupal.org/node/45111 and http://drupal.org/node/1181250). Specifically, the actual serious bug would be in whatever contrib or custom code triggers the error in the first place; the issue here is that Drupal core doesn't deal with that error very well, but that doesn't seem like a serious bug in Drupal core itself.

If you disagree and feel this must still be marked major, please provide a very specific justification and keep in mind that every major issue prevents other features that people are working hard on from getting into Drupal core, due to the issue queue threshold policy (until/unless #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is addressed, at any rate). Note that I am not singling out this issue for particular attention but rather (slowly) trying to go through the issue queue and do this in many places so that other features still have a chance to get into Drupal core soon without the thresholds getting in the way. I am trying my best to be impartial, and of course, just because an issue isn't marked "major" certainly doesn't mean it's unimportant to work on. You can follow my progress via the tag I've just added to this issue :)

Sorry for the noise; I'm told that slashes in tags can break the autocomplete on drupal.org.

Updating summary to include specific details

Ouch. This issue can be absolutely brutal if it bites you.

The trigger in my case was an update hook which was creating taxonomy terms, with the pathauto module in use:

1. taxonomy_term_save()
2. pathauto_taxonomy_term_insert() (i.e. hook_taxonomy_term_insert)
3. pathauto_taxonomy_term_update_alias()
4. pathauto_create_alias()
5. _pathauto_set_alias()
6. _pathauto_path_is_callback()
7. menu_get_item()
8. menu_rebuild() -- for every term saved.

Once I'd tracked down the cause, my workaround was to set $GLOBALS['conf']['menu_rebuild_needed'] = FALSE; in my update hook before the terms are created, and then unset($GLOBALS['conf']['menu_rebuild_needed']); afterwards.

(It would be nice if I didn't have to remember to check for this in future any time an update hook seems to be running slowly, however.)

n.b. The excessive menu rebuilds may also have been killing memcached on my dev server, but I'm not certain of this. It did happen several times over the course of debugging the issue, however.

Can we revert this issue back to Drupal 7?

I can't even find any of the relevant code in Drupal 8!

menu_get_item() doesn't seem to exist in D8 any more, and grepping for other bits of context from the patch didn't generate any hits either.

So, not a D8 bug?

On the basis of D8 change record Removed menu_get_item() and menu_set_item() confirming that the problematic function no longer exists in D8, I'm reverting this to a D7 issue.

FWIW, while there have been various comments in this issue pointing the finger at contrib code, it's not clear to me that the code which triggered the problem in my case (see #35) was doing anything wrong.

To me this does seem like a genuine core bug, which can cause serious performance problems, and ought to be dealt with.

This is a duplicate of #2425259: Router rebuild lock_wait() condition can result in rebuild later in the request (race condition), which explains all cases where we have race conditions and problems, while the patch here is only taking one case into account.

I just committed #2425259: Router rebuild lock_wait() condition can result in rebuild later in the request (race condition) to Drupal 7, but I did a bit of testing and I don't think it solves the problem here (or at least doesn't completely solve it).

I tried to set up an artificial scenario like what I ran into in #21, by just inserting a broken query at the top of the try{} statement in menu_rebuild(), e.g.:

db_query("broken")->fetchField();

Then I added some logging and visited the front page of the site to see how many times it was trying to do a rebuild per page request.

Either with or without #2425259: Router rebuild lock_wait() condition can result in rebuild later in the request (race condition) it was still attempting multiple rebuilds per page request. But with the patch in #25 of this issue, it only attempted one rebuild per page request.

One could argue that menu_get_item() should rebuild the menu caches to ensure that there are no problems so that code does not arbitrarily break when the menu caches are cleared. (e.g. saving any view on a production instance)

In that case it would be: Works as designed

Drupal 8 decided to not go into this direction and rather put the pressure on the deployment instead.

However with #25 the masks could still be empty from a failed attempt and the same problem with multiple rebuilds is still present.

I am not sure how likely it is that the menu rebuild code breaks completely.

Here is a re-roll of the patch in #25 as it is no longer applying to 7.x.

Patch from #45 helps to avoid issues when l() is used in hook_menu() item 'description' when variable_set('theme_link', FALSE); is not used.

Global question: hook_menu() docs state that "description" is "The untranslated description of the menu item.". Doesn't this mean that l() is prohibited because it accept "The translated link text for the anchor tag." as the link text?

I think that in addition to patch from #45 we should patch hook_menu() doc block to explicitly point developers not to use l() or use it in conjunction with variable_set('theme_link', FALSE); wrapper as @zhangtaihao suggested in his comment https://api.drupal.org/comment/18019#comment-18019.

I have to apologize, I totally missed that menu_get_item() is not protected against recursion in itself for rebuild ...

That is a very very tricky scenario, as what do we return then? 404? Looking at the half-build data?

I think the best is to ensure the theme registry cannot be loaded during router rebuild and we ourselves used

l() => theme_link => FALSE

to fix it, but it indeed is tricky as e.g. some hook_menu() like to call ctools plugins to load routes from other things, then this loads other things and that can somewhere call l() again, which calls the registry, which calls menu_get_item() ...

Stil even a simple check if we are currently rebuilding in l() for the case of theme would likely be enough for hardening 90% of sites ...

I've run into these sorts of issues on a few sites.

The issue I was facing was that once menu_rebuild has been called while running updates, calls to most bits of the menu system will trigger a menu_rebuild again.
This means that updates take ages to run!

Attached is a patch that keeps the intention of the code in core, in that it just schedules a menu_rebuild for the next page request, rather than doing it the next time menu_get_item is called.

I think this should work fine, as it's essentially marking the data that was built as dirty, but not trying to repeatedly build the same data over and over.

#49 Very interesting approach. I wonder if we should not always do it on the next page request instead of making MAINTENANCE MODE really really slow ...

I think #49 is worth discussing, but not sure it helps with the rest of this issue. Those of us who experienced a problem here before saw it on regular page requests (not in MAINTENANCE_MODE)...

Old, related issue with a similar approach to #49, possibly a duplicate.

Has this issue been fixed in another thread?

Is it related to this issue: Optimize menu_router_build() / _menu_router_save(), which appears more recent?