Allow needs_destruction services to run on page cache hits

This is not explicit test coverage, but it's the test coverage I was a trying to add when I discovered this bug. The green test run shows there's no explicit or implicit test coverage for the current behaviour at all.

OpenTelemetryFrontPagePerformanceTest::testNodePageHotCache() has some kind of problem with the page cache not being set - this is what led me to here in the first place. However I can't reproduce that problem locally, only on gitlab CI, which makes it hard to track down what's going on. I've added some extra assertions to the Umami front page test instead, which should fail on a test only run.

@znerol pointed out in slack the original reason this was done: https://www.drupal.org/project/drupal/issues/2348679#comment-9517741 →

All event subscribers to the onKernelTerminate event will be run if we do my original approach here - and many of those aren't and shouldn't be run on page cache hits (like path alias subscriber).

However, we we take service destruction out of the onKernelTerminate event subscriber and run it in its own right, that might work. Services only get destructed if they're already initialized, so that's minimal overhead to add.

I've pushed a commit that does that.

This seems to work - but we either need to rename that class or just move the logic into DrupalKernel since it leaves it wrongly named.

This requires adjusting some test assertions - possibly because the collector is running after other services in kernel destruct and hence picking up more database queries.

In general I think while the performance test use case is very specific, this change in behaviour is an improvement:

1. Destructable services should destruct as late as possible - i.e. after other things that might happen in kernel destruct like UserRequestSubscriber or automated cron.

2. If you tag a service with needs_destruction it seems reasonable to assume it'll be destructed on any request where it's iniatialized.

Inlining was causing very strange errors on gitlab which I couldn't reproduce with local test runs, so backed that out of the MR. We could just rename the class/service and leave it as is, if this approach is otherwise OK.

There any concern about the increase in calls?

It will only run for services that were already instantiated and are also destructable, which will usually be nothing.

In that case let’s give it a shot

We need to rename the class to something other than KernelDestructionSubscriber to avoid confusion before commit (and put it in a different namespace). Maybe something like ServiceDestructor?

I had tried that before, but completely failed - however @alexpott pointed out that this was because I hadn't removed the call to the event subscriber (now a non-existing class), because it's so low level the test failures weren't obvious and I was looking for bugs with what I'd changed, not what I hadn't changed. With that cruft removed, it's fine. This avoids the renaming issue altogether.

+1 for me. kernel.destructable_services seems like a good name and feedback appears to be addressed.

Going to leave in review for additional eyes.

Been a few days, going to go ahead and mark.

Added feedback to the MR, also we need to remove the event subscriber from core.services.yml:

  kernel_destruct_subscriber:
    class: Drupal\Core\EventSubscriber\KernelDestructionSubscriber
    calls:
      - [setContainer, ['@service_container']]

Should address #16, also added a CR for the deprecation.

Appears deprecation feedback has been addressed.

Needs rebasing following changes to StandardPerformanceTest.

Rebased. A few assertions have to be increased, which I'm pretty sure is because it's recording more things that were already happening.

@catch maybe instead service should be deprecated? https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... →

@andypost all event subscribers are implicitly @internal per https://www.drupal.org/about/core/policies/core-change-policies/bc-polic... → The service is only to register the event subscriber so similarly we should remove rather than deprecate here - we'd have to remove the event subscriber tag anyway.

@catch Clear, thank you!

back to RTBC

Committed e5fc18c and pushed to 11.x. Thanks!

longwave → closed merge request !6040

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

Created a major bug as a result of this issue: 🐛 Container compile crash when a service decorates a destructable service Needs work