On automatic deployment for a project hosted on multiple servers with redis and varnish, we bump into a race condition issue when we add a new contrib module in the composer.json and enable it in the same commit.
The import failed due to the following reasons:
Unable to install the [CONTRIB_MODULE] module since it does not exist.
What happens is that during the deploy steps the caches are filled (after getting cleared) from the production website which doesn't yet contain the new module in its codebase. drush cim is then performed on code that expects this module.
drush pmu -y [CONTRIB_MODULE]mv web/modules/contrib/[CONTRIB_MODULE] [CONTRIB_MODULE]drush crmv [CONTRIB_MODULE] web/modules/contrib/[CONTRIB_MODULE]drush cim -yThe problem is in core/lib/Drupal/Core/Config/ConfigImporter.php because on config import, drush doesn't explicitly perform a ->reset() on the module list. On drush enable for example, drush does explicitly reset the module list before grabbing it.
Will attach patch in comment.
Reviewing.
| Comment | File | Size | Author |
|---|---|---|---|
| #15 | interdiff_14-15.txt | 742 bytes | ravi.shankar |
| #15 | 3312001-15.patch | 665 bytes | ravi.shankar |
| #14 | drupal-race-condition-on-config-import-3312001-14.patch | 664 bytes | stijndmd |
| #12 | drupal-race-condition-on-config-import-3312001-12.patch | 664 bytes | stijndmd |
| #9 | interdiff_6-9.txt | 905 bytes | ravi.shankar |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #2
stijndmd commentedAttached a patch which makes sure the module list is reset immediately before performing the extension updates of the config import.
Comment #3
stijndmd commentedComment #4
catchI've seen similar during local development, usually it's because I forgot a step changing branches, but I think the reset would save a drush cr every so often.
I think this might be a trivial enough fix we could commit it without test coverage, however we should at least add a comment explaining the situation we're fixing.
Comment #6
stijndmd commentedSomething like this is enough or do we need a longer explanation + issue link?
Comment #7
stijndmd commentedComment #8
longwaveI ran into this exact issue today during a deployment.
I think we should keep the original line as-is and add
$this->moduleExtensionList->reset();before with an explicit comment as to why we are resetting. Otherwise, without test coverage, we risk losing the->reset()in a future refactor.Comment #9
ravi.shankar commentedMade changes as per comment #8, please review.
Comment #10
longwaveI think that's fine. It's hard to explain the race condition in a succinct comment, but the git log will point back to this issue if anyone wants to read more.
Comment #11
catchMaybe something like:
Reset the module list in case a stale cache item has been set by another process during deploymentfor the comment?Comment #12
stijndmd commentedChanged the comment.
Comment #13
longwaveThat works for me. Thanks!
Comment #14
stijndmd commentedHere's one with a correction since the previous didn't apply
Comment #15
ravi.shankar commentedFixed the issue of line closing before 80 characters, this should be good to to now.
Maintaining the same status RTBC.
Comment #17
catchCommitted/pushed to 10.1.x, cherry-picked back through to 9.4.x, thanks!