Registry rebuild does not work with non core lock.inc

Closing as duplicate of #1832394: "drush rr" fails if @settings.php, lock_inc => memcache/memcache-lock.inc

Please use dev version for now to avoid this known issue.

FYI: sites/all/modules/contrib *is* supported in this workaround, but sure, it could be improved.

You probably mean a code like this?

  if ($conf['lock_inc'] && file_exists($conf['lock_inc'])) {
    $cache_lock_path = $conf['lock_inc'];
  }
  elseif ($conf['lock_inc'] && file_exists(DRUPAL_ROOT . '/' . $conf['lock_inc'])) {
    $cache_lock_path = DRUPAL_ROOT . '/' . $conf['lock_inc'];
  }
  else {
    $cache_lock_path = $include_dir . '/lock.inc';
  }

Unfortunately, it will not work, since we operate initially on just the DRUSH_BOOTSTRAP_DRUPAL_DATABASE level.

But if you have any suggestion on how to improve current workarounds, patches are welcome.

I found that issue after posting #1993506: Memcache Storage breaks "drush rr".

Here's a patch that should solve the problem for Drupal 7 in a generic way in addition to the hack introduced by #1832394: "drush rr" fails if @settings.php, lock_inc => memcache/memcache-lock.inc.

+    $cache_lock_path_tmp = DRUPAL_ROOT . '/'. variable_get('lock_inc', 'no_custom_lock_inc');

Do not use custom empty value for 'lock_inc' variable. We can make patch for D7 better. It is attached.

Removed extra whitespaces.

Patches in #6 an #7 will create a fatal error about undefined functions if you move the module that contains the lock.inc and use drush rr to register that movement!
In that case the variable is still set to the old value but the require_once will be skipped!

My patch in #5 should deal with that situation.

I've tested my patch with Memcache Storage and drush rr command. There was no errors.

Did you move Memcache Storage module in a different directory after installation and activation of lock.inc and run drush rr to detect that movement?

Yep. And it works fine for me. Take a look at our patches - they are almost similar. The only difference is that you use incorrect empty value for lock_inc variable. All other code is the same. So I don't understand how my patch can throw an error while your works with no problems.

There is one more thing I don't like in #5 patch: if user pointed incorrect path to the lock handler Drush will use core's one (includes/lock.inc). But this is wrong behavior. I mean that if user wants to use another handler for locking mecanism we should support him. And if path to file is wrong then Drush should throw an exception instead of using core's lock.inc (as Drupal does, see _drupal_bootstrap_variables()).

The idea to check variable lock_inc sounds interesting, if possible, even if it is an expensive check - you must bootstrap Drupal high enough to be able to read it, and since you are using this tool probably because you already *can't* bootstrap Drupal high enough (wsod etc ), it is for the same exact reason unreliable (if not almost useless), as you have already realized probably, because you *still* have to check if the file really exists there, because, well, the variable may be cached and the cached value incorrect (at least we must assume that it is cached), so it can't be used as a final check. This means that variable check only helps (a bit) to *guess* where the file is expected, but we can't rely on this information.

Two more notes:

+++ b/registry_rebuild.drush.inc
@@ -57,26 +57,25 @@ function drush_registry_rebuild() {
+  if (drush_drupal_major_version() >= 7) {

This part is incorrect for Drupal 8 because there are no variable lock_inc at all.

+++ b/registry_rebuild.drush.inc
@@ -57,26 +57,25 @@ function drush_registry_rebuild() {
+    // For Drupal 6 we only search for some well known lock implementations in
+    // common directories.
     $cache_lock_path = $include_dir . '/lock.inc';
+    if (file_exists(DRUPAL_ROOT . '/sites/all/modules/memcache/memcache-lock.inc')) {
+      $cache_lock_path = DRUPAL_ROOT . '/sites/all/modules/memcache/memcache-lock.inc';
+    }
+    elseif (file_exists(DRUPAL_ROOT . '/sites/all/modules/contrib/memcache/memcache-lock.inc')) {
+      $cache_lock_path = DRUPAL_ROOT . '/sites/all/modules/contrib/memcache/memcache-lock.inc';
+    }
+    elseif (file_exists(DRUPAL_ROOT . '/sites/all/modules/redis/redis.lock.d6.inc')) {
+      $cache_lock_path = DRUPAL_ROOT . '/sites/all/modules/redis/redis.lock.d6.inc';
+    }
+    elseif (file_exists(DRUPAL_ROOT . '/sites/all/modules/contrib/redis/redis.lock.d6.inc')) {
+      $cache_lock_path = DRUPAL_ROOT . '/sites/all/modules/contrib/redis/redis.lock.d6.inc';

This is seems to be odd too. We should use same algorithm as in bootstrap of Drupal 6 (see _drupal_bootstrap()).

So I propose new patch for this issue.

@omega8cc,

The lock_inc variable shouldn't be cached as it is set only from settings.php. Even if this varible was cached (unlikely, but we should check this too), when admin wants to set another handler for locks he should add this configure line to the settings.php file, and this will override cached value. So I don't see any problems here.

@omega8cc you bootstrap to Database. At this Point the configuration in settings.php has been loaded already. The common case is to set lock.inc in settings.php.

@Spleshka you're right about the default value. But the file exists check is required.

OK, so to sum this up:

1. The only check we can rely on is a file_exists()
2. Checking the lock_inc variable may offer a hint, to avoid blindly hardcoded if/else chain of file_exists(), which is a plus.
3. Checking the lock_inc variable must use a proper fallback and not a dummy "nowhere" and should assume that its value is unreliable (hence #1).

If you could re-roll the patch and confirm that it works both for D6 and D7 (both for drush and php script method), that would be great!

I'm insist that we shouldn't provide fallback. If user set wrong path to lock.inc file in settings.php then bootstrap should fail!

Good to see here drupal_set_message "wrong path" without fail, cause this looks much more friendly despite php fail

EDIT +1 for fallback

Ok, I see that I'm alone here :) Attached a patch with fallback support.

Any updates here? I've tested this on Drupal 7, everything works fine.

I had a deeper look at d6. I think there's no action required at all, because in newer d6 versions already load the configured lock.inc during DRUSH_BOOTSTRAP_DRUPAL_DATABASE.

So we only need the manual loading for d7 and probably d8.

Based on my initial patch I attached a new one that removes the d6 part, includes Spleshka's input and adds a TODO for d8.

@mkalkbrenner,

Drupal 7 loads lock.inc file during bootstrap too. But here we don't invoke drupal_boostrap() at all. Instead of this we provide here partial implementation of bootstrap. So we should load lock.inc for both Drupal 6 and Drupal 7 (and other major versions too).

Please, review #19.

@Spleshka sorry, you're wrong. We bootstrap to the database level! Read the code:

function drush_registry_rebuild() {
  ...
  drush_bootstrap_to_phase(DRUSH_BOOTSTRAP_DRUPAL_DATABASE);
  ...
}

function drush_bootstrap_to_phase($max_phase_index) {
  ...
  drush_bootstrap($phase_index, $max_phase_index);
  ...
}

function drush_bootstrap($phase, $phase_max = FALSE) {
  ...
  $phases = _drush_bootstrap_phases(TRUE);
  ...
  $current_phase = $phases[$phase_index];
  ...
  $current_phase(); // calls _drush_bootstrap_drupal_database()
  ...
}

function _drush_bootstrap_drupal_database() {
  drush_log(dt("Successfully connected to the Drupal database."), 'bootstrap');
  drupal_bootstrap(DRUPAL_BOOTSTRAP_DATABASE);
}

And now the difference between Drupal 6 and 7. Drupal 6 loads lock.inc during DRUPAL_BOOTSTRAP_DATABASE:

function _drupal_bootstrap($phase) {
  ...
  switch ($phase) {
    case DRUPAL_BOOTSTRAP_DATABASE:
      // Initialize the default database.
      require_once './includes/database.inc';
      db_set_active();
      // Allow specifying alternate lock implementations in settings.php, like
      // those using APC or memcached.
      require_once variable_get('lock_inc', './includes/lock.inc');
      lock_init();
      break;
   }
   ...
}

Drupal 7 introduced a new bootstrap phase called DRUPAL_BOOTSTRAP_VARIABLES which happens after DRUPAL_BOOTSTRAP_DATABASE!

function _drupal_bootstrap_variables() {
  global $conf;

  // Initialize the lock system.
  require_once DRUPAL_ROOT . '/' . variable_get('lock_inc', 'includes/lock.inc');
  lock_initialize();

  // Load variables from the database, but do not overwrite variables set in settings.php.
  $conf = variable_initialize(isset($conf) ? $conf : array());
  // Load bootstrap modules.
  require_once DRUPAL_ROOT . '/includes/module.inc';
  module_load_all(TRUE);
}

Due to the fact that lock.inc is loaded during the new phase, we have to deal with that for Drupal 7.

But doing anything for Drupal 6 makes no sense because core loads lock.inc earlier. So please have a look at the patch in #21. For Drupal 7 it's identical with your patch in #19.

Yes, you are right here. So for me this patch is RTBC.

Thanks. #21 is RTBC.

Patch #21 works for me.

I will test this more in our BOA environment and then commit as soon as possible. Thanks everyone!

This probably needs to get in and a new release real soon now. Just hit this myself.

confirmed that path #21 works for D7

#21 committed: http://drupalcode.org/project/registry_rebuild.git/commit/5669774

Also ported for registry_rebuild.php: http://drupalcode.org/project/registry_rebuild.git/commit/816cdd0

Thanks everyone!

Thanks! Are you ok with doing a release as well? That way those who do "drush dl" will get this.

Done! https://drupal.org/node/2056519

Thanks! omega8cc++

Thanks so much for your care for this little helper.

@rfay This 'little helper' is a very important lifesaver. Thank YOU! :)

So sad to say, this still does not work with memcache (7.x-1.0) and Drupal 7.22

drush rr
PHP Fatal error:  Cannot redeclare lock_initialize() (previously declared in /var/www/.../git_working/includes/lock.inc:67) in /var/www/.../git_working/sites/all/modules/contrib/memcache/memcache-lock-code.inc on line 19

I didn't pay careful attention to the issue in general, so may have missed something important, but it looks like this isn't resolved, at least for this site.

Ouch, so I was right when I thought this needs more time and testing. Sadly, I didn't have time to test this yet (I'm still on a vacation with family - yeah, I know..) and I trusted others here to report correct results and then was motivated to go ahead. Sorry. You can trust only your own tests :/

@rfay How you have defined the path in the lock_inc? I can only guess it is absolute, so this will not work:

$cache_lock_path = DRUPAL_ROOT . '/'. variable_get('lock_inc', 'includes/lock.inc');

This is because we are making here a silent assumption about the path in lock_inc being relative to Drupal root, which is not guaranteed.

Nope, we're using a relative path

$conf['lock_inc']         = 'sites/all/modules/contrib/memcache/memcache-lock.inc';

OK, but we should support also absolute path anyway. Plus, we should use only single fallback there. Not sure if that is the problem here, but please try the patch attached.

Or we simply have to bootstrap to DRUPAL_BOOTSTRAP_VARIABLES in D7 instead of DRUPAL_BOOTSTRAP_DATABASE, because anything we do by trying to access lock_inc is done too early?

The only problem: there is no such thing in Drush API as DRUSH_BOOTSTRAP_DRUPAL_VARIABLES

Shucks. Yeah, your patch, as is, does not resolve this.

The original idea was just to get to the lowest (and least likely to fail) bootstrap level and fix things there. So I'm sure DRUPAL_BOOTSTRAP_DATABASE was chosen only for convenience.

Yeah, so maybe we can call drupal_bootstrap(DRUPAL_BOOTSTRAP_VARIABLES) directly to get there? No idea if this will work with Drush in general, but will work with script, of course. Untested patch for Drush part attached.

By the way, I have no idea how this check for lock_inc worked for anyone without bootstrapping to DRUPAL_BOOTSTRAP_VARIABLES in D7.

#43 still has the same fail :-(

DRUPAL_BOOTSTRAP_CONFIGURATION should be enough...

Uh, the last patch is silly, we would need to load bootstrap.inc first.

To elaborate: Since lock_inc needs to be defined in the settings file to work (as lock is used when fetching the variables from the db anyway) just boostrap to the configuration level and then $conf['lock_inc'] will be available. Variables is too far because the first thing variables does is initialize the lock system....

So I think there is in fact problem in D7 core:

/**
 * Loads system variables and all enabled bootstrap modules.
 */
function _drupal_bootstrap_variables() {
  global $conf;

  // Initialize the lock system.
  require_once DRUPAL_ROOT . '/' . variable_get('lock_inc', 'includes/lock.inc');
  lock_initialize();

  // Load variables from the database, but do not overwrite variables set in settings.php.
  $conf = variable_initialize(isset($conf) ? $conf : array());
  // Load bootstrap modules.
  require_once DRUPAL_ROOT . '/includes/module.inc';
  module_load_all(TRUE);
}

Did you notice that that nice "Load variables from the database, but do not overwrite variables set in settings.php" comes after lock system is already initialized? Too late.

That simply means then by bootstrapping to this phase we can' fix anything, and we will hit the same wall we are trying to avoid, because, well, how can we override core here? We can't.

Yep, that seems to be the case here. But does DRUPAL_BOOTSTRAP_CONFIGURATION is really enough? We can try.

OK, but how bootstrapping only to DRUPAL_BOOTSTRAP_CONFIGURATION will help if we bootstrap also to DRUPAL_BOOTSTRAP_FULL a bit later anyway? Or to put it in a different way, what is the advantage of using DRUPAL_BOOTSTRAP_CONFIGURATION over DRUPAL_BOOTSTRAP_DATABASE if none of them hits DRUPAL_BOOTSTRAP_VARIABLES anyway?

I will follow up on another vacation-drupal-window, but maybe someone smarter than me will figure this out in the meantime?

Removing the tag for now.

omega8cc++

Incredible responsiveness.

omega8cc++:

By the way, I have no idea how this check for lock_inc worked for anyone without bootstrapping to DRUPAL_BOOTSTRAP_VARIABLES in D7.

... because DRUPAL_BOOTSTRAP_CONFIGURATION calls drupal_settings_initialize() which loads the conf array from settings.php. And DRUPAL_BOOTSTRAP_CONFIGURATION is included in DRUPAL_BOOTSTRAP_DATABASE.

I'm convinced that the patches after #21 will not fix rfay's issue described in #35. (But dealing with an absolute path is a valid addition.)

@rfay: Can we debug your setup together?

BTW we run drush rr with the patch #21 on every deployment on more than 10 sites without any problems.

Oh dear. I have caused much pain and do sincerely repent.

Unknown to me, I had a version of registry_rebuild in the /usr/share/drush/commands directory, overriding everything else I did.

I learned lots of things, including remote CLI debugging.

But the most important thing I learned is that

sudo drush dl registry_rebuild

(which I didn't actually know I did, but must have)

puts code into commands instead of .drush.

The release version of the code works fine for me.

@omega8cc re #48:

(TLDR version: lock_inc is special, and DRUPAL_BOOTSTRAP_VARIABLES is probably the best choice for the preferred bootstrap level because it's the most consistent early level.)

It is on purpose that lock.inc is processed before the variables are copied from the db. This is because the variables subsystem requires the lock subsystem to prevent a race condition that can lead to corrupted variables, IIRC.

lock_inc is one of the few "special" variables that is used in early bootstrap and as such ONLY works when defined from settings.php. (If you aren't using page_cache_without_database then it IS the only "special" variable other than page_cache_without_database. The reason is that not using page_cache_without_database causes DRUPAL_BOOTSTRAP_PAGE_CACHE (level 1) to be equivilent to DRUPAL_BOOTSTRAP_VARIABLES (level 3) because it recursively bootstraps to level 3 in the middle of level 1. page_cache_without_database is used for high performance caching situations where you want to serve the entire request without going higher than level 1.)

For the purposes of being able to recover from the most number of issues (including problems in bootstrap modules), this is the process of doing an absolute minimal bootstrap in D7, without any automatic loading whatsoever:

* Bootstrap to configuration level (0)
* Make sure $_COOKIE[session_name()] is set to something so the early page cache doesn't try to take over.
* Set $conf['page_cache_without_database'] to TRUE to disable the recursive bootstrap to level 3 from level 1.
* Bootstrap to database level (2)
* Manually do all but the last line of _drupal_bootstrap_variables().
At this point the database and variables are up, but no modules are loaded.

HOWEVER, I can't think of a valid use case for doing this. I don't think module_load of bootstrap modules will fail barring syntax errors. I could be mistaken though.

IMO the correct bootstrap level for registry rebuild purposes is DRUPAL_BOOTSTRAP_VARIABLES. This ensures that variables are picked up even if the user is using page_cache_without_database. The bootstrap order for this will be either CONFIGURATION, PAGE_CACHE (->DATABASE, ->VARIABLES),SKIP, SKIP or CONFIGURATION, PAGE_CACHE, DATABASE, VARIABLES.

@rfay Ah! :)

@mkalkbrenner and @bdragon - Thanks for the detailed explanations. I must admit that my vacation-mode is not compatible with situation like this, and exposes shameful logical flaws :) That said, I think that since we have to use DRUPAL_BOOTSTRAP_DATABASE anyway before we do things like cache_clear_all(), there is no practical reason to use DRUPAL_BOOTSTRAP_CONFIGURATION initially, unless we want to make the process that "atomic" with as low bootstrap as possible also during the check for lock_inc value. The reason I think we don't need DRUPAL_BOOTSTRAP_VARIABLES is that (besides it makes it further D7 specific) we don't need it at the as-low-as-possible initial stage, and because a moment later we go to DRUPAL_BOOTSTRAP_FULL anyway (because we run the fixes more than once, internally) -- I guess this would require running rebuilds/cache clears 3x instead of 2x, as it is now.

Keep in mind that I'm still in a vacation-mode, so if you think I just wrote something stupid, don't hesitate to correct me :)

hi
i lost all of the day by searching and nothing found
i check catch tables , use registry rebuild , ...
finally i start checking problem manually

my problem solved

export registry table from fresh d7 in REPLACE mode and IMPORT it to my WSOD

i think registry rebuilt modules work buggy

Hello, this does not seem solved with latest version.
In fact, "$cache_lock_path_absolute = variable_get('lock_inc');" can't work because the global conf variable has not been set yet. I don't really know how to fix this without bootstraping variables.

To fix this, I've added a constant at the top of the file :

define('DRUPAL_LOCK_FILE', DRUPAL_ROOT . '/profiles/foundation/modules/contrib/memcache/memcache-lock.inc');

And then :

if (function_exists('registry_rebuild')) { // == D7
  $cache_lock_path_absolute = variable_get('lock_inc', DRUPAL_LOCK_FILE);
  if (!empty($cache_lock_path_absolute)) {
    $cache_lock_path_relative = DRUPAL_ROOT . '/'. variable_get('lock_inc', DRUPAL_LOCK_FILE);
    // Ensure that the configured lock.inc really exists at that location and
    // is accessible. Otherwise we use the core lock.inc as fallback.
    if (is_readable($cache_lock_path_relative) && is_file($cache_lock_path_relative)) {
      $includes[] = $cache_lock_path_relative;
      print "We will use relative variant of lock.inc.<br/>\n";
    }
    elseif (is_readable($cache_lock_path_absolute) && is_file($cache_lock_path_absolute)) {
      $includes[] = $cache_lock_path_absolute;
      print "We will use absolute variant of lock.inc.<br/>\n";
    }
    else {
      print "We will use core implementation of lock.inc as fallback.<br/>\n";
      $includes[] = DRUPAL_ROOT . '/includes/lock.inc';
    }
  }
  else {
    $includes[] = DRUPAL_ROOT . '/includes/lock.inc';
  }
}

@Renrhaf
Thanks for the snippet. We had the same problem with memcached causing a lock error. When applying your suggestions from #62 to the registry_rebuild.php file it all works great.

Awesome job!

@Renrhaf -- I think it was explained by @mkalkbrenner at https://www.drupal.org/node/1971684#comment-7723577 and then also by @bdragon

TLDR; DRUPAL_BOOTSTRAP_CONFIGURATION (level 0) calls drupal_settings_initialize() which loads the conf array from settings.php, so lock_inc is known at this stage, if exists there. It is a special variable and is not designed to be stored/accessed in/from the database, hence we don't have to bootstrap to DRUPAL_BOOTSTRAP_VARIABLES to access it. We can access it directly after we have bootstrapped to DRUSH_BOOTSTRAP_DRUPAL_DATABASE / DRUPAL_BOOTSTRAP_DATABASE (level 4 in Drush and 2 in Drupal).

If this doesn't work for you, it may need further investigation, but it should work if you have it defined in the settings.php file.