STOP the registry integrity constraint violation nightmare

There's no lock around registry rebuild - do we want to consider adding one instead of the db_merge()? That might not be a good idea but came to mind looking at the patch.

If anyone needs to reproduce this issue, it happens on named sites (not using sites/default) when registry paths are registered for one site (ie. sites/d7.localhost/modules/contrib/feeds/plugins/A.inc) and then we try to clean the cache in a cloned Drupal installation with a different sites location (ie sites/d7B.localhost).

A quick sample of failure can be created with the steps listed at http://drupal.org/node/1347894#comment-5366800.

Once the registry has recorded paths, Ctools attempts to re-register them on hook_registry_alter by picking a cached version that has an inexistent path (see #1371700: CTools adds cached plugins with wrong paths during hook_registry_files_alter() in multisite systems), causing the integrity error.

subscribing, very annoying indeed on D7 sites!

Please stop subscribing by posting a comment and click on the "Follow" link at the top of the issue.

I backported the patch to 7.x and can confirm that the integrity error gets fixed and the registry table ends in a consistent state after cleaning the cache (hence rebuilding the registry). However, I had to remove the line

+      $newquery = clone $query;

so the values could be merged correctly. As it was, the same Integrity constraint error raised.

Attached are an updated patch for D8 and its backport to D7. This fixes quite a bit or issues in Drupal 7 contributed modules that had minor fixes such as:

• #1347894: Clear cache causes integrity constraint violation
• #1028728: Integrity constraint violation: 1062 Duplicate entry 'DateAPITestCase-class'
• #1324000: Failed to rebuild registry on PDO Exception: duplicate entry
• #1265564: PDOException due to class auto-loading registry when running a cache clear
• #1371700: CTools adds cached plugins with wrong paths during hook_registry_files_alter() in multisite systems
• #1224498: PDOException
• #1283208: PDOException: SQLSTATE[23000]

If anyone wants to recreate an environment where this happens, please contact me on my personal contact form and I will share a Dropbox folder where I have got a Drupal core and database dump configured to test this particular error when cleaning the cache.

How do we know for certain the registry_update() will complete in less than 30 seconds?

There's code in _registry_parse_files() that currently does this, before _registry_parse_file() is called:

// Delete registry entries for this file, so we can insert the new resources.
db_delete('registry')
  ->condition('filename', $filename)
  ->execute();

If we're changing _registry_parse_file() to use db_merge(), can the above code be removed?

I modified the d8 patch to work for d7 (s/core\//), and this got things working again on a multisite dev. environment. I did not review for commenting on #14. Just a "it fix it" review for D7.

I was having a PDOException: SQLSTATE[23000]: error on a site managed through aegir and couldn't clone or migrate the site because of that. I manually applied the changes in the patch at #10 to my 7.12 platform which fixed the PDOException.

So essentially i'd agree that it works on a multisite d7. If anyone can roll a d7 version of the patch, that would be great for future reference.

Thanks for the patch and thanks to @juampy for your messages in the forums on all sorts of module issue queues looking for the cause of the PDOexception.

B

You are welcome!
Just to give some perspective of how this bug affects other modules: for me it all started with an Aegir site too (thus, using the multi site feature). I first saw an error related to Feeds #1347894: Clear cache causes integrity constraint violation when clearing out the cache, then I debugged it further and went down to CTools #1371700: CTools adds cached plugins with wrong paths during hook_registry_files_alter() in multisite systems and finally ended up in core at this issue. It seems that it can be solved at different levels but definitively core fixes them all.

@sun: why would we want a lock here? Updating/Rebuilding the registry is a rare task, usually when caches are cleared or modules are enabled/disabled. The lock comes with some problems:

• as already stated by catch the lock time limit can be exceeded, then we can have a parallel registry update again anyway.
• Administrator race condition: One administrator clears caches, another one enables a module in the meantime. So the second call to registry_update() just waits and assumes that first call has included the new files of the module. This might not be the case and we get an inconsistent registry state, where a module is enabled but its files are not in registry.
• code complexity: adding a lock on top of registry updates makes this code even more hard to grasp.

So we should just make the registry update more robust, db_merge() seems perfect for that.

@David_Rothstein: yes, the deletion part beforehand seems useless then.

New patch without the lock and without the obsolete delete query.

Drupal 7 patch.

I thik this is rtbc...

Yes, I agree it's RTBC. I am a big db_merge() fan and this is a good place to use it.

I don't get it, request #2 arrives after #1, so the changed module information should be present already?

Anyway, I would argue that the rebuilding race condition is a separate, non-critical issue. As the issue title says, let's fix the nightmare first.

After discussion with sun on IRC and more thinking, here is the patch with the lock again and with the obsolete db_delete() removed. The lock helps to eliminate the race condition.

I did a little manual testing with an artificial sleep(20); delay during the lock in registry_update(). Initiating a cache clear in one request and enabling a module in a parallel request worked as desired. The second request updates the registry twice: once in module_enable() and once through drupal_flush_all_caches() at the end of system_modules_submit(). So even if after the first update the registry is inconsistent, this is cleared up by the cache clear at the end again. I also tested parallel module enabling with drush 5.x, also works as expected. So no worries anymore from my side.

After more thinking about the lock I came to the conclusion that we have to rebuild the registry after waiting on an older, parallel request. This ensures that after a call to module_enable() the registry is always in a consistent state. Patch attached.

Regarding deletion: classes/interfaces can get removed from a file, if we don't remove those entries they stay as stale entries forever. Example: Add an empty class CommentFoo to comment.entity.inc. Clear caches, CommentFoo now shows up in the registry table. Remove CommentFoo from comment.entity.inc. Clear caches, CommentFoo should now be gone from the registry table. As a matter of fact, I just tested that and the class is not gone, so this patch is not ready yet.

Some further reasoning: we want module_enable() to be atomic and fully consistent. If there is a registry update from an old request going on we have to initiate another registry update to be sure that possible changes are accounted for. Otherwise the outcome of module_enable() could be inconsistent. This is not a problem for Drupal core as my tests have shown (extra cache clearing is done), but any other code that relies on module_enable() could cause problems when the registry is not up to date.

I hoped this would be explained by the comments in the code, but it is tricky to describe :-(

Ok, for whatever reason I must have made a mistake during my testing of the registry entry deletion. It works as advertised.

Here is an updated patch that contains code comments as suggested by beejeebus on https://gist.github.com/1918695

Fixed the array duplication.

I have not removed the double lock acquiring, as I think it is important for an accurate registry state. Why are the consequences of this approach not clear? Let me explain in other words: if a registry update is currently going on and other requests also want to update the registry, they line up in the lock and wait until the old update is finished. Then an arbitrary request picks up the new update and the others just have to wait it out. The state of enabled modules is directly queried from the system table in _registry_update(), so anything that has been changed until this particular point in time will be incorporated in the registry update.

Just because we do not use this anywhere else in core does not mean that this is not reasonable.

Urgs, the previous patch contains an unrelated change in comment.entity.inc from my manual testing. Removed that.

Minor issue:

+  $in_installer = defined('MAINTENANCE_MODE') && MAINTENANCE_MODE == 'install';

This should probably just use drupal_installation_attempted().

Right, fixed that.

Sad, but I agree at this point. Let's just fix this now and deal with the minor race condition later.

The double lock means the process could end up waiting for at least 60 seconds, since max_execution_time can often be 30 seconds we could end up erroring out rather than continuing anyway, so agreed it'd make sense to tackle this in a separate issue.

Rerolled, now with the single lock_wait() in registry_update() again.

Tagging.

Added an issue summary.

drupal8.registry-rebuild-nightmare.0.patch queued for re-testing.

Added issue summary.

Here is a port of #45 for Drupal 7. I used it to verify that it fixes #1371700: CTools adds cached plugins with wrong paths during hook_registry_files_alter() in multisite systems, which is a registry constraint error that happens during rebuild.

Was the follow-up issue created yet? I am not sure how to word that one myself.

Committed/pushed to 8.x, moving to 7.x for backport.

Reuploading #49, no commit credit.

+++ b/includes/bootstrap.incundefined
@@ -3080,11 +3080,30 @@ function registry_rebuild() {
+ **

This needs fixing.

Also, this hunk is missing from the reroll:

+    // Delete any resources for this file where the name is not in the list
+    // we just merged in.
+    db_delete('registry')
+      ->condition('filename', $filename)
+      ->condition('name', $matches[2], 'NOT IN')
+      ->execute();

Re-rolled.

Looks identical

is #55 a patch i can apply to a running 7.12 system?

@denjell - The patch should apply to 7.x-dev. 7.12's codebase has slight differences so you would want to test it. If the patch passes committer review, it will also be included in the next point release of Drupal 7.

ok thanks. any idea when the next point release of 7 is going to roll out?

Wednesday. These things get announced at http://groups.drupal.org/core.

Committed and pushed to 7.x. Great work! I had actually tested this bug against CTools a few weeks ago while it was still NR and it worked for me as well. This seems worth mentioning in the release notes.

I am using Drupal 7.14 and still get this error when trying to add a new node!

Please copy/paste the exact error you're seeing.