Updated: Comment #N

Problem/Motivation

DrupalKernel contains code that attempts to support multiple webservers for the same site, by detecting module changes and rebuilding the container if it changed.

That code a) only supports a very limited set of possible situations where this could happen (like pushing new code with different services or adding a language) and b) Affects performance for all the sites that don't use multiple webservers for no reason.

Proposed resolution

Remove it.

We have ideas to solve this problem in a better way, like comparing a flag in the database and one in a file and rebuild on mismatch. This can be done outside of core, optionally, to avoid the overhead for sites that don't need it.

Remaining tasks

User interface changes

API changes

Files: 
CommentFileSizeAuthor
#21 interdiff.txt1.3 KBznerol
#21 2228215-remove-module-check-mini-21.diff7.38 KBznerol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,653 pass(es).
[ View ]
#17 interdiff.txt2.43 KBznerol
#17 2228215-remove-module-check-mini-17.diff7.36 KBznerol
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,212 pass(es).
[ View ]
#16 interdiff.txt860 bytesznerol
#16 2228215-remove-module-check-mini-16.diff4.92 KBznerol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,227 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#14 interdiff-6-14.txt2.94 KBznerol
#14 2228215-remove-module-check-mini.diff4.67 KBznerol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,417 pass(es), 1,063 fail(s), and 86 exception(s).
[ View ]
#10 interdiff.txt3.46 KBznerol
#10 remove-module-list-check-2228215-10.patch7.6 KBznerol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,775 pass(es), 261 fail(s), and 9 exception(s).
[ View ]
#7 interdiff.txt2.39 KBznerol
#7 remove-module-list-check-2228215-7.patch4.13 KBznerol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,091 pass(es), 35 fail(s), and 0 exception(s).
[ View ]
#6 remove-module-list-check-2228215-5.patch1.94 KBznerol
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,298 pass(es), 1,081 fail(s), and 80 exception(s).
[ View ]
#1 remove-module-list-check-2228215-1.patch1.92 KBBerdir
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,038 pass(es), 1,037 fail(s), and 70 exception(s).
[ View ]

Comments

Berdir’s picture

Status:Active» Needs review
StatusFileSize
new1.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,038 pass(es), 1,037 fail(s), and 70 exception(s).
[ View ]

Here's the patch. Installer worked.

catch’s picture

Yes this should be something you can opt-in to for sites that want the fail safe - and there's other ways to accomplish it (like timestamp comparison) than loading the configuration every request.

We already rebuild the container on switches to/from multi-lingual and based on settings.php changes, and this doesn't cover either of those, so it's a false sense of security.

sun’s picture

       $this->registerNamespaces($this->container->getParameter('container.namespaces'));

Why do we still need this?

Berdir’s picture

Because otherwise the module namespaces aren't registered, ever :)

I first removed that, but it completely falls apart :)

Status:Needs review» Needs work

The last submitted patch, 1: remove-module-list-check-2228215-1.patch, failed testing.

znerol’s picture

StatusFileSize
new1.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,298 pass(es), 1,081 fail(s), and 80 exception(s).
[ View ]

Reroll.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new4.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,091 pass(es), 35 fail(s), and 0 exception(s).
[ View ]
new2.39 KB

Just an idea: Use two separate flags allowDumping and allowLoading. Forcibly set the latter to FALSE from within updateModules. If we already know that the container on disk is out of date, do not bother trying to load it...

Status:Needs review» Needs work

The last submitted patch, 7: remove-module-list-check-2228215-7.patch, failed testing.

catch’s picture

Priority:Normal» Major
znerol’s picture

Related issues:+#2033567: Remove TestBase::rebuildContainer()
StatusFileSize
new7.6 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,775 pass(es), 261 fail(s), and 9 exception(s).
[ View ]
new3.46 KB

Now that the module-list is not checked anymore during the bootstrap, it is essential that the container is dumped each time it is modified in the simpletest parent site. Therefore we need to prevent WebTestBase::rebuildContainer() from creating a kernel without $allowDumping, or even better remove that method entirely (as per #2033567: Remove TestBase::rebuildContainer()).

znerol’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 10: remove-module-list-check-2228215-10.patch, failed testing.

The last submitted patch, 6: remove-module-list-check-2228215-5.patch, failed testing.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new4.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,417 pass(es), 1,063 fail(s), and 86 exception(s).
[ View ]
new2.94 KB

Let's not attack #2033567: Remove TestBase::rebuildContainer() and instead try to keep the changes small. Interdiff is against #6 (the reroll of the original patch).

Status:Needs review» Needs work

The last submitted patch, 14: 2228215-remove-module-check-mini.diff, failed testing.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new4.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,227 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new860 bytes

Reintroduce the idea that after DrupalKernel::updateModules(), the container never ever should be loaded from disk anymore during subsequent rebuilds of the kernel. This also leads to the container being dumped (if dumping is enabled) after each DrupalKernel::updateModules().

znerol’s picture

StatusFileSize
new7.36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,212 pass(es).
[ View ]
new2.43 KB

Fix DrupalKernelTest.

The last submitted patch, 16: 2228215-remove-module-check-mini-16.diff, failed testing.

sun’s picture

Status:Needs review» Reviewed & tested by the community

I'm glad that you removed the changes related to #2033567: Remove TestBase::rebuildContainer(), because I strongly disagree with the idea of removing it. — Instead, what we need is an enhancement to PhpStorage that allows it to work in a "single hashed file mode"; i.e., the class name of the dumped file gets a hash appended, which in turn would enable us to actually reload the already dumped container of the test child site from disk (which we cannot do right now, since the class name of the service container is always identical, and PHP does not support to unload code, so rebuildContainer() intends to just reload, but actually has to rebuild, because the class is loaded already).

Aside from that remark, this looks good to me.

As a directly related follow-up, we should get back to #2160091: drupal_rebuild() rebuilds container twice, since drupal_flush_all_caches() also rebuilds it

Lastly, not sure what the consequences of this patch are with regard to #1897468: Kernel rebuilds service container on every request when a module vanishes

catch’s picture

Status:Reviewed & tested by the community» Needs work

Couple of comment issues, otherwise looks great.

  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -171,12 +178,16 @@ class DrupalKernel implements DrupalKernelInterface, TerminableInterface {
    +   *   (optional) FALSE to prevent that the kernel is attempted to be read from

    'to prevent the kernel attempting to read the dependency injection container from disk'?

  2. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -344,6 +355,12 @@ public function updateModules(array $module_list, array $module_filenames = arra
    +    // disable loading of a dumped kernel from the disk, because it is

    Are we dumping the kernel or the container?

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new7.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,653 pass(es).
[ View ]
new1.3 KB
sun’s picture

Status:Needs review» Reviewed & tested by the community

Thanks!

catch’s picture

Status:Reviewed & tested by the community» Fixed

Committed/pushed to 8.x, thanks!

  • Commit 341b903 on 8.x by catch:
    Issue #2228215 by znerol, Berdir: Remove module check in DrupalKernel.
    
alexpott’s picture

The original commit contained #2259275: Rename "DrupalUnitTestBase" to KernelTestBase so I reverted and recommitted see 486e276 (the revert) and 3cc312e (the proper commit)

  • Commit 3cc312e on 8.x by alexpott:
    Issue #2228215 by znerol, Berdir: Remove module check in DrupalKernel.
    
  • Commit 486e276 on 8.x by alexpott:
    Revert "Issue #2228215 by znerol, Berdir: Remove module check in...
znerol’s picture

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.