menu_masks variable is empty (race condition)

looks good to me, and auto-recovers when you get into a nasty situation of your site going permanent WSOD.

Why should this happen in the middle of an API function?

As an alternative, can we set the variable that says a rebuild is needed and maybe hard code a few masks in for this just this page request?

how about this? A quick test shows it works if the masks are deleted/emptied.

+1 to #1 nice patch, also fixes #307937: MYSQL Syntax error on installation with pre-filled tables
-1 to #7, that just looks ugly...

how about RTBC on the #1 patch?

I've been bitten by this bug. Subscribe. Will review.

I can't currently install D7 because of this, which is seriously impeding my ability to commit patches. ;) It'd be nice of chx/pwolanin could have a discussion about the best way to proceed.

webchick, did applying #1 fixed it for you?

It's happening for me every time I try to install D7 after the DBTNG patch. Would be good to find what is causing the issue, for what reason the menu rebuild does not happen during a fresh installation. Subscribing.

I've had this on occasion but not been able to consistently reproduce.

People have seen this on D6 and D7, so it's some kind of race condition I think. But I'm at a loss as to what.

Actually, the D7 code has a flaw that's partly fixable... let me go find it.

some of the D7 problems may be fixed by this: http://drupal.org/node/238760#comment-1023094

chx: see #307937: MYSQL Syntax error on installation with pre-filled tables for a way to forcefully reproduce this bug...

@alexanderpas: I can not reproduce the bug with the instrucctions there, the first step is "instal site normally", how is that if I can not even install Drupal. The error appeared during the installation process.

I think this is a simpler way to achieve what chx's patch does in #1. With this patch in place, I can variable_del the menu masks via a PHP node and life goes on normally. Without the patch, Drupal dies until I hit update.php or otherwise manually force a menu rebuild.

Patch is against 6.x, though applies to 7.x also with offset.

with added code comment

even better!

marked #280015: cannot install drupal - menu_router error as duplicate.

Ok, I committed this. I've run into this bug before, and it's good to have a fix in. However, I'd LOVE to figure out WHY this happens so we can fix the underlying issue. Talking to Peter and chx though it doesn't sound like it's so easy to determine since it's very erratic.

Moving down to 6.x for consideration.

Seconded webchick's comment on the fact that it would be better to know the underlying cause. Committed to 6.x now as a stop-gap solution.

Since you have marked #307937 as a duplicate of this issue, I'll let you know that:
- I have tried installing Drupal 6.6, downloaded recently on WAMP 2.0c
- during the installation, I get the MySQL syntax error described in #307937
- if the installation is successful (happened once), after the site times out on anything, the menu isn't completely built

My cookies are enabled and PHP memory limit is set to 128Mb.

Anyone confirmed that the problem is completely fixed? Judging by posts elsewhere, it is not.

Thanks.

See:
#2946: Login fails and no warning is issued if cookies are not enabled
#234539: Critical SQL error in installer with {menu_router}

This bug is still present in the most recent release version of Drupal 6. It doesn't happen every time, but I've had it occur twice: once with 6.4 and once with 6.19, both on Ubuntu 10.10. Cookies are definitely enabled. The first time it happened I was installing from the O'Reilly Using Drupal pre-packaged archive. I found a posting from someone who reported having solved the problem by replacing that archive with the latest production release of Drupal 6. So I pulled down 6.19 and tried again. Same problem. I found another posting that said installation had failed using Chrome as the browser but succeeded with Firefox. So I cleared out the settings.php file and the database and tried again using Firefox. This time it succeeded. However, I was skeptical that the browser could actually be the problem so I did some additional testing using fresh downloads of 6.19 and was able to get successful installations using either browser. So it's much more likely to be a timing problem. Yes, I know that makes the bug harder to reproduce, and therefore harder to fix. But it's still a bug which hasn't been fixed. If you do some googling, you'll see plenty of postings from frustrated users struggling with the bug well after this issue was marked as "closed (fixed)" two years ago. I've changed the "Version" field from "6.x-dev" to "6.19"; there's no indication in this interface whether the "Version" field is intended to refer to the version in which I'm reporting the problem, or a target version for the fix. Apologies if I guessed wrong. If the setting to "6.x-dev" was intended to mean "we fixed this in the development branch two years ago but we have no intention of ever deploying the fix in the released version of the system" then the bug is in the process, not the code. It's hard to believe the Drupal development community would really expect all of its users to dig through the bug database and apply all of the approved patches. And I'd also like to think that comment #26 isn't just being ignored.

I ran into this one just now while I was attempting an install of Open Atrium. I figured it was an issue with Atrium's install profile, until I tried to do a "standard" installation and failed there too. When I searched d.o, I saw #280015: cannot install drupal - menu_router error. Comment #40 mentioned the system date. I was installing onto a VMware guest that had been suspended for a long time, and ntpd had died at some point. My system date was about 40 days in the past. Once I resynced my system time, the install went off with no problems.

The Error

User warning: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near ') ORDER BY fit DESC LIMIT 0, 1' at line 1 query: SELECT * FROM menu_router WHERE path IN () ORDER BY fit DESC LIMIT 0, 1 in _db_query() (line 169 of includes/database.mysqli.inc).

How to trigger error:
2 requests come in at exactly the same time and menu_masks variable is not defined. menu_execute_active_handler() runs for both of them. In both cases the menu_masks variable doesn't exist thus menu_rebuild() will run on both. The first request to get a lock will complete fine. The request that doesn't get a lock will wait and not run the _menu_router_build() function; this function sets the menu_masks variable, but in the request that didn't acquire the lock, the variable is still not defined. Thus later on when menu_get_ancestors() is ran, the foreach loop (in this function) is not ran and we get 2 empty arrays returned causing the SQL query to blow up on us inside of menu_get_item().

Possible Solutions:

• Have $write_database = FALSE like we do in module_rebuild_cache() and system_theme_data() and duplicate work.
• Read from the database for the menu_masks variable and set it in the $conf global (thus allowing variable_get to work correctly in this request).
• If the menu_masks variable is empty provide an array that goes from 255 down to 1.
• Have an alternate foreach loop in menu_get_ancestors that checks for a mask everywhere.

My Opinion:
I think we should read from the database and set the menu_masks variable after lock_wait runs inside of menu_rebuild. If for some reason that variable doesn't exist we will still be ok because inside of menu_get_ancestors we will have an alternate foreach loop that doesn't require this variable. Patch attached doesn't contain the alt menu_get_ancestors logic, only a place holder for it.

Thoughts?

Removed the menu_get_ancestors placeholder logic as that could be causing issues.

Man I am happy we are have now surrounded this bug after 8 months of digging. Nice work Mike.

Good news, I think you in the right place.
Bad news, applied patch didn't correct our issue.

Good news, we see this a couple times per day randomly across our network of sites so there are plenty of chances to test the approach in #37 or nail down a better approach to #36.

Thanks for the detective work on this!

Here's an alt approach that is similar to #7. We will test and report back if this fixes the issue for us. I don't feel like pulling apart the bitwise math in the foreach loop so I'm trying the alt option here. Yeah it slightly slower but this happens about once every 500k requests for us. Patch attached.

#39 fixes this bug on our sites.

Code wise the array needs to start from 255 and count backwards. 255 comes from the fact that the path column in the menu_router table is varchar(255). Something like this would work instead of the hard coded array if we want to go that route.

array_reverse(range(1, 255));

Patch against D8, D7 & D6 using chx's advice from #42

Man I wish the test bot was a little smarter. Now with only the D8 patch attached. D7 & D6 patches attached in #43

This test might fail on your patch.
MenuRouterTestCase::testMenuGetItemNoAncestors

@chx
Is the default parameter getting removed in D8 for variable_get? If not, what is the advantage of your way? In either case it's doing what it should so I will yield to CHX on this point. Calling this RTBC.

The difference with #46 is that we avoid calling range() on every request - instead that will only happen if nothing is returned from variable_get() - chx pointed this out to me in irc when I asked the same thing, it's easy to forget the variable_get() default actually gets called either way.

That's a good reason to add a comment here though explaining why it's doing that.

Thanks :)

Committed/pushed to 8.x, moving to 7.x for backport. Backport should be straightforward so marking novice for that.

Patches for D7 & D6 based off of #50. The D6 patch also takes care of some white space issues.

7.X patch is exactly the same :)

Damn. Nice sleithing! Committed and pushed to 7.x for parity with 8.x. That should also take care of the critical.

However, this comment doesn't explain the code below it. Or at least not to me.

+++ b/includes/menu.incundefined
@@ -321,7 +321,14 @@ function menu_get_ancestors($parts) {
+  // If the optimized menu_masks array is not available use brute force to get
+  // the correct $ancestors and $placeholders returned. Do not use this as the
+  // default value of the menu_masks variable to avoid building such a big
+  // array.
+  if (!$masks) {
+    $masks = range(511, 1);

How does range(511, 1) "use brute force to get the correct $ancestors and $placeholders returned"? Can we clarify this comment more? We need something along the lines of #40-#42. range() normally goes "little to big," and it's not clear what significance the number 511 has.

I'm using this on D6, but I changed
$masks = range(511, 1);
to
$masks = range($end, 1);
since values larger than $end will be ignored anyway in the foreach loop.

I think I've explained this better and I've also used the tip from abautu in #56

The 511 comes from (2^9) - 1. 9 comes from MENU_MAX_PARTS.

Patches for D8, D7, and D6. The D6 patch also takes care of some white space issues.

Needs issue summary

Updated issue summary.

Updated issue summary.

No longer needed in D8; RouteProvider::getCandidateOutlines() seems like this is where the code landed and it uses $end instead of 511 so this change has been incorporated in D8.

Patch from #57 applies cleanly to latest D7 so this is essentially the same patch. Will post the D6 one once this goes green.

#356399: Optimize the route rebuilding process to rebuild on write is related and specifically fixes on race condition where the variable gets empty after a lock_wait() on menu_get_item(), this might be a duplicate.

Should this be a duplicate or should we re-open #356399: Optimize the route rebuilding process to rebuild on write for D7?

#65 still applies to current 7.x.