Always free up old container on rebuild, not only in the installer

Pushed the change to an MR.

I did stumble into this again because we're running into issues when our current large update, which installs and uninstalls several modules through update functions, gets killed due to excessive memory. In local testing in ddev without memory limit, I observed up to 666MB (yes, exactly, really). With this change, it did grow to 250-300MB but then suddenly dropped again to < 200MB and never went above 300.

Tests are still failing pretty hard, to be investigated.

Yes, one problem is the uninstallValidators in ModuleHandler. That isn't being reattached in \Drupal\Core\Extension\ModuleInstaller::updateKernel, so it references the old container. The problem is that accessing it is deprecated:

The "module_installer.uninstall_validators" service is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Inject "!tagged_iterator module_install.uninstall_validator" instead. See https://www.drupal.org/node/3432595

Not sure how to solve that :-/

another, more common one is the mime type guesser, a static call that happens through \Drupal\Core\File\MimeType\MimeTypeGuesser::registerWithSymfonyGuesser(), this currently only on preHandle(). I tried moving it to rebuildKernel as well, but didn't fix the test fail yet.

@godotislate: Thanks, that helped. Not sure if what I came up is the best approaach, but it works. At first it didn't, but then I noticed that that failing test was a config importer, so that also hit #3564735: Container / service exceptions while installed from existing config. added the same change here which I came up with separately.

Also, as usual with these changes, there are rabbit holes within rabbit holes. The failing package manager tests took me a while to track down. Noticed that the event dispatcher gets persisted there, which I finally tracked down to \Drupal\Tests\package_manager\Kernel\SandboxManagerBaseTest::register(). That's not ok, because it has injected service locators and other callbacks that keep references to the old container around. Disabled for now, lets see what really fails without that and then we probably want a separate issue to handle that differently, or do it here depending on how big that gets.

Still a ton of test fails here. Lots of additional package tests doing weird stuff with persisted services. Various one-off tests having pseudo-DI properties that become outdated. updating is broken, request stack is empty. Maybe #2613044: Requests are pushed onto the request stack twice, popped once will help with that, maybe not. More likely that the update kernel doesn't do things properly.

I debugged this a bit, my guess is that this happens in updates when a container rebuild happens, then the controller and injected dependencies are still on the old container.

I honestly don't know if this is feasible like this. Interestingly, when running updates through drush, I didn't have issues like this, I'm not sure how to resolve this. We could try to reinject in Drupal\system\Controller\DbUpdateController::handle() after running the updates.

I wonder if we should resort to a different mechanism to fix the memory issue. Maybe we could could have a soft reset, that only resets non-synthentic services?

> This change looks like it should result in quite a few changes to \Drupal\Core\DrupalKernel::resetContainer() as that is re attaching synthetic services like the kernel. Don't think it needs to with this change.

I don't quite get what you mean with that.

I didn't write a new comment, but I did implement that soft-reset approach that only resets non-synthentic services, which is working a lot better. So yes, with that, we possibly no longer need some of the changes, like the module installer as it would still kind of work, it's still wrong though. Same with the package manager test, I can probably undo that tag, but those tests and how they change the container are really weird.

I figured out what caused the test to fail, it was that I didn't reset private services.

It's so hard to follow all this, but I feel like doing this could allow us to simplify and remove some overlapping bits in \Drupal\Core\DrupalKernel::attachSynthetic() and \Drupal\Core\DrupalKernel::resetContainer(), I don't entirely get why they're two different methods and both are called during the installer and the reset only if more than one module is being installed?

Currently, the reset() method only keeps synthetic services. But then there's also the persist_ids parameter and services tagged with that, which is set in resetContainer(). If we'd also respect that directly inside reset() then we wouldn't need that anymore, I think? We still need to reinitialize some services though.

But I think that could go into follow-ups.

My last cleanup there didn't handle the weird request_stack service correctly (non-synthetic service but must not be reset)

I thought I replied to @21 but can't seem to find it anymore.

That call is in the super-critical-path (pre-page-cache-hit). Triggering an event there is not an option.

That said, the reason it's there isn't that it is earlier, it is that rebuilding a container does re-register that, which currently is inconsistent (still on the old container).

Setting back to RTBC as I just accepted some suggestions.

We have a follow-up: #3612177: Update autoload classmap for Symfony runtime and etc.. I can add the MimeTypeGuesser, will try my patch on core and see what it reports, we are likely missing a lot.