Add Service termination API to reliable terminate/shut down services in the container

Copying over my comment from #512026-134: Move $form_state storage from cache to new state/key-value system:

any reason why we're not using Symfony\Component\HttpKernel\TerminableInterface? That's used by the Kernel for bundles that need termination already. Not sure whether it would be legit, but we could possibly fake/mock a Request and Response, in case there is none (e.g., in tests).

In light of that, at minimum, the name of Symfony's interface is also more correct for an interface. AFAIK, interfaces should always be *able or similar.

Related to #5, what will we want to do with the 'http_kernel' service when DrupalKernel is rebooted (due to module_enable() calling updateModules() on it)? Currently in HEAD, we let it get garbage collected along with all the other services, but we don't explicitly call terminate() on it, and then a new one gets instantiated in the rebuilt container. If we want to call terminate() on it before it gets destroyed, a problem we'll run into is that we don't have a $response yet. Perhaps it would be better to tag it with 'persist' so that the same 'http_kernel' instance persists into the rebuilt container? But then, what should we do with 'needs_termination' services that aren't also tagged with 'persist'? Presumably, we would need to terminate() them, but again, without a $response, and if so, then we need it to be our own interface, not Symfony's.

I don't think it makes sense at all to use Symfony's TerminableInterface - it is currently used exclusively by the HttpKernel and its decorators (Kernel and HttpCache) for the purpose of dispatching the very event upon which our services need to act. The terminate() method takes a request and response so it can dispatch a kernel event terminating the request/response cycle. We'd have to pull these out of the $event in our listener in order to pass them on to our services needing termination even though none of them currently have any use for them... That seems pretty crazy to me.

So it sounds like what we actually need is a ShutdownableInterface (or something) that just has a shutdown() method. Then we can call that method on all known shutdownable objects from the terminate event, OR from other places where it makes sense.

Rebooting/building/placing a kernel does *not* invoke terminate (maybe it should?) so persist and needs_termination should not conflict.

There's the 'kernel' service (DrupalKernel) and the 'http_kernel' service (HttpKernel). When the 'kernel' service is rebooted, it's still the same instance, so terminate() isn't and shouldn't be called on it. However, the 'http_kernel' service is not currently tagged as 'persist'. I'm not yet clear on whether we should persist it or not, but allowing it to get destroyed and replaced with a new instance without calling terminate() on the old instance seems potentially problematic. What we decide on that is relevant, because it's the http_kernel that fires the KernelEvents::TERMINATE event that we're subscribing to for terminating 'needs_termination' services.

Suppose we choose to persist 'http_kernel' and therefore, not call terminate() on it during a 'kernel' reboot. Then, any services tagged 'needs_termination', but not tagged 'persist', will get destroyed and replaced with new instances without terminate() being called on the old instances. Wouldn't that get us back to the same problem this issue is meant to solve (PHP garbage collection happening in unpredictable order)? I think we can solve that though by making DrupalKernel call terminate() itself on all such services.

I think figuring out this reboot flow can be punted to a follow up, but I bring it up, because I think it provides an additional reason for doing what this patch does in terms of introducing its own interface rather than using Symfony's TerminableInterface.

Berdir: I'm suggesting exactly what the issue summary says: Some services have a shutdown, or cleanup, or commit, or whatever method. Those need to get called at termination, but we don't want services to listen to terminate themselves because that couples them to being event listeners. So instead we have a single terminate listener that calls shutdown on all of those services.

Oh, now that read the patch, so it is! :-) My concern is that reusing the "terminate" name may be confusing; didn't we say above that the kernel terminate event is not synonymous with "we need to call this cleanup method on services"? And there's already a \Symfony\Component\HttpKernel\Terminatable interface. That's going to be needlessly confusing.

I'm not at all wedding to shutdown() as a name; I just think it should be something that won't be confused with the existing Symfony interface. Otherwise the patch above looks fine to me.

I can see these options:

1. The most simple + most concrete + most obvious: DestructableInterface + public function destruct(). That's what we're actually replacing here; i.e., __destruct() » destruct().
2. OTOH, I already wondered whether we're perhaps supposed to override the Kernel::shutdown() method? CoreBundle::shutdown() gets already called from there.
3. We could add a tag to all container services that need destruction, or we could also iterate over all and check instanceof DestructableInterface or ShutdownableInterface (although I don't think that's a name).
4. In any case, we'd have to ensure that the new shutdown() / destruct() methods will be invoked for both HttpKernel terminations AND manually enforced Kernel shutdowns. Speaking of, I briefly looked, but wasn't able to figure out where and when or how Kernel::terminate() or HttpKernel::terminate() is actually registered as a PHP shutdown function... is terminate() invoked at all currently? :)

The symfony approach seems to be to not trust PHP's shutdown at all (understandable), and rely on the front controller to call the terminate method on the kernel, which happens after the request is set. Basically it is our drupal_register_shutdown_function() equivalent, but with an in-this-case-undesirable HTTP object dependency.

Sleeping over this, I'd recommend:

1. Drupal\Core\Kernel\DestructableInterface: public function destruct()
2. Iterate over all instantiated services, instead of tagging them.
3. Make CoreBundle::shutdown() trigger the destruction stack, or override Kernel::shutdown() directly.
4. Make DrupalKernel::terminate() trigger the destruction stack, after parent::terminate().
5. Additionally register DrupalKernel::shutdown() as a standalone PHP shutdown function. Essentially the replacement for drupal_register_shutdown_function(). If this gets erroneously invoked in tests, there must have been a fatal error or similar to begin with. Instead of fixing those, we should make Lock + whatever fail-safe enough to not break horribly in case they aren't triggered on shutdown.
6. That said, a DrupalKernel::shutdown() can always be complemented with a DrupalKernel::NoShutdownPlease() to set an internal flag on DrupalKernel to not trigger the destruction stack. More or less what drsf() is currently doing. (Current tests are copy/swapping-out the registered list of functions upon tearDown(), but I seriously doubt that this would ever swap-back-in previous/original shutdown functions. Thus, just making sure to kick out a previously registered stack that wasn't destructed should be sufficient.)

Overall the approach here looks solid. Some comments, though: (Leaving as CNR for testbot, but these need to be addressed.)

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterServicesForDestructionPass.php
@@ -0,0 +1,46 @@
+    // Reverse the order of tagged services to destruct them in the opposite
+    // order of which they are defined.
+    // @todo: Does this need a better system to detect references?
+    $services = array_reverse($container->findTaggedServiceIds('needs_destruction'));

The order in which services are defined in the DIC is entirely arbitrary. Who knows what people may put into their bundles. Why does the order matter? And how can we make it not matter, because I foresee that being a major point of failure?

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterServicesForDestructionPass.php
@@ -0,0 +1,46 @@
+      $refClass = new \ReflectionClass($class);
+      $interface = 'Drupal\Core\DestructableInterface';
+      if (!$refClass->implementsInterface($interface)) {
+        throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface));

We don't do this sort of protection anywhere else in the DIC. I don't think we need to here, especially since it means loading all distructable classes in order to do so. We can just leave this out.

+++ b/core/lib/Drupal/Core/EventSubscriber/KernelDestructionSubscriber.php
@@ -0,0 +1,74 @@
+  /**
+   * Invoked by the terminate kernel event.
+   *
+   * @param \Symfony\Component\HttpKernel\Event\PostResponseEvent $event
+   *   The event object.
+   */
+  public function onKernelTerminate(PostResponseEvent $event) {
+    $this->destructIniatedServices();
+  }
+
+  /**
+   * Destructs registered services if necessary.
+   */
+  protected function destructIniatedServices() {
+    foreach ($this->services as $id) {
+      // Check if the service was initialized during this request, destruction
+      // is not necessary if the service was not used.
+      if ($this->container->initialized($id)) {
+        $service = $this->container->get($id);
+        $service->destruct();
+      }
+    }
+  }

Why separate these? Especially as destructInitiatedServices (which is misspelled) is protected, it's useless elsewhere. This code is trivial enough to just go in onKernelTerminate.

+++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/ServiceDestructionTest.php
@@ -0,0 +1,53 @@
+  /**
+   * Verifies that services are correctly destructed.
+   */
+  public function testDestruction() {
+    // Enable the test module to add it to the container.
+    $this->enableModules(array('bundle_test'));
+
+    // The service has not been destructed yet.
+    $this->assertNull(state()->get('bundle_test.destructed'));
+
+    // Get the service destruction.
+    $service_destruction = $this->container->get('kernel_destruct_subscriber');
+
+    // Simulate a shutdown. The test class has not been called, so it should not
+    // be destructed.
+    $response = new Response();
+    $event = new PostResponseEvent($this->container->get('kernel'), $this->container->get('request'), $response);
+    $service_destruction->onKernelTerminate($event);
+    $this->assertNull(state()->get('bundle_test.destructed'));
+
+    // Now call the class and then invoke the kernel terminate event again.
+    $this->container->get('bundle_test_class');
+    $service_destruction->onKernelTerminate($event);
+    $this->assertTrue(state()->get('bundle_test.destructed'));

This is in DUTB, not WebTest. Please break it up to 2 separate test methods. Please. With sugar on top.

Most of Crell's points are things I noticed aswell, so +1 for those. Just a few more doc nits...

+++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/RegisterServicesForDestructionPass.phpundefined
@@ -0,0 +1,46 @@
+   * Implements CompilerPassInterface::process().

Does this need the full namespace?

+++ b/core/lib/Drupal/Core/EventSubscriber/KernelDestructionSubscriber.phpundefined
@@ -0,0 +1,74 @@
+ * This destructs iniated services tagged with needs_destruction

typo here.

+++ b/core/lib/Drupal/Core/EventSubscriber/KernelDestructionSubscriber.phpundefined
@@ -0,0 +1,74 @@
+   * Registers a a service for destruction.

Just one a is fine :)

+++ b/core/modules/system/lib/Drupal/system/Tests/DrupalKernel/ServiceDestructionTest.phpundefined
@@ -0,0 +1,53 @@
+    // Get the service destruction.

This comment sounds weird.

+++ b/core/modules/views/lib/Drupal/views/ViewsDataCache.phpundefined
@@ -241,25 +242,18 @@ public function fetchBaseTables() {
+  public function destruct() {

I would like to get some test coverage to actually test this is cached as expected in ViewsDataCache. But this is definitely a follow up for me to get on with and not something that should be tackled in this issue.

Thanks for resurrecting this, @Berdir.

The @todo about the destruction order is concerning. AFAICS, to destruct all services in the right order of dependencies, we'd have to implement Graph to determine the actual dependencies, based on the service Definitions that are passed via References. I wonder whether there isn't a concept for that in ContainerBuilder already?

Berdir: :-P

My statement in the other issue was suggesting that the service that implements "Terminate me!" have two methods: "I will need to do cleanup" and "clean me up".

My statement above is that the event listener that is calling all of the various things that need to be terminated doesn't need to split the loop out of the listener method itself. It's still simple glue code either way.

A test method should test one and only one thing. I've been very consistent on that for years. :-) We frequently violate that in WebTest because of how expensive it is to re-initialize WebTest for a new test method, but that doesn't mean it's not still wrong to do so.

Let's leave out all of the ordering and dependency stuff for now, including the array_reverse() call. That is only useful if we assume that services in CoreBundle are in dependent order, and I don't think that's even true now.

It's not a hard requirement, I suppose. Thinking about it further it may not make sense here. It just seemed odd to have a lot of if-wrapped methods.

Found a few more doc issues and fixed them in the patch attached. I believe all the concerns have been adressed in the patch from #37, so RTBC.

This looks good to me. We'll have to revisit the dependencies, but it's not making anything worse than using __destruct(). Committed/pushed to 8.x.