[FrameworkBundle][HttpKernel] Add DI tag for resettable services #24155

derrabus · 2017-09-11T14:40:21Z

Q	A
Branch?	3.4
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#23984
License	MIT
Doc PR	TODO

This PR uses #24033 to introduce a DI tag for resettable services.

TODO after merge:

Add an interface, make the "method" attribute optional and enable autoconfiguration.
Consider adding a config option to enable/disable this feature.
Configure leaking services of the core bundles as resettable.

stof · 2017-09-11T14:48:06Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ResettableServicePass.php

+class ResettableServicePass implements CompilerPassInterface
+{
+    const RESET_TAG = 'kernel.reset';
+    const METHOD_ATTRIBUTE = 'method';


We don't add constants for them in Symfony

Tag name is now a parameter, attribute name has been inlined.

stof · 2017-09-11T14:49:20Z

src/Symfony/Bundle/FrameworkBundle/EventListener/ServiceResetSubscriber.php

+    {
+        foreach ($this->services as $id => $service) {
+            foreach ($this->resetMethods[$id] as $method) {
+                call_user_func(array($service, $method));


$service->$method() would be faster by avoid the call_user_func frame in the stack

nicolas-grekas

sweet :)

nicolas-grekas · 2017-09-11T14:50:25Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ResettableServicePass.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\DependencyInjection\Compiler;


I propose to move the pass and the event subscriber in HttpKernel, to make it easier to reuse.

Good idea. In this case, tag name and method attribute should be constructor parameters of the pass, I guess.

for the tag name yes, that's what we do usually,
for the attribute name, no need IMHO

nicolas-grekas · 2017-09-11T14:51:33Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ResettableServicePass.php

+/**
+ * @author Alexander M. Turek <me@derrabus.de>
+ *
+ * @internal


no need for this

nicolas-grekas · 2017-09-11T14:52:14Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ResettableServicePass.php

+class ResettableServicePass implements CompilerPassInterface
+{
+    const RESET_TAG = 'kernel.reset';
+    const METHOD_ATTRIBUTE = 'method';


the two const should be removed (for consistency at least: we don't use consts for tags in any pass yet)

nicolas-grekas · 2017-09-11T14:52:58Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ResettableServicePass.php

+
+            foreach ($tags as $attributes) {
+                if (!isset($attributes[self::METHOD_ATTRIBUTE])) {
+                    throw new \RuntimeException(


should be on one line

nicolas-grekas · 2017-09-11T14:54:11Z

src/Symfony/Bundle/FrameworkBundle/EventListener/ServiceResetSubscriber.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\FrameworkBundle\EventListener;


HttpKernel namespace

nicolas-grekas · 2017-09-11T14:54:17Z

src/Symfony/Bundle/FrameworkBundle/EventListener/ServiceResetSubscriber.php

+ *
+ * @author Alexander M. Turek <me@derrabus.de>
+ *
+ * @internal


nicolas-grekas · 2017-09-11T14:54:52Z

src/Symfony/Bundle/FrameworkBundle/EventListener/ServiceResetSubscriber.php

+    /**
+     * {@inheritdoc}
+     */
+    public static function getSubscribedEvents()


should be moved last, after onKernelTerminate

nicolas-grekas · 2017-09-11T14:55:51Z

src/Symfony/Bundle/FrameworkBundle/EventListener/ServiceResetSubscriber.php

+    {
+        foreach ($this->services as $id => $service) {
+            foreach ($this->resetMethods[$id] as $method) {
+                call_user_func(array($service, $method));


or just $service->$method();

nicolas-grekas · 2017-09-11T14:56:24Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

@@ -117,6 +118,7 @@ public function build(ContainerBuilder $container)
        $container->addCompilerPass(new CachePoolPrunerPass(), PassConfig::TYPE_AFTER_REMOVING);
        $this->addCompilerPassIfExists($container, FormPass::class);
        $container->addCompilerPass(new WorkflowGuardListenerPass());
+        $container->addCompilerPass(new ResettableServicePass());


~~as PassConfig::TYPE_AFTER_REMOVING?~~

nicolas-grekas · 2017-09-11T14:59:49Z

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Compiler/ResettableServicePass.php

+                    );
+                }
+
+                $methods[$id][] = $attributes[self::METHOD_ATTRIBUTE];


Do we really need to allow several methods? I'm not sure I see a use case where a class would require two calls to be resetted, do you?

I don't, but I did not see a reason to forbid it. Also, I would need to limit the kernel.reset tag to max one tag per service then.

In such cases, we usually just ignore the next tags and always take index 0.
We should do the same here IMHO.

nicolas-grekas · 2017-09-11T16:06:52Z

About the todo:

Add an interface, make the "method" attribute optional and enable autoconfiguration.

I suggest doing so in another PR

Add a config option to enable/disable this feature.

I suggest not adding this at all

Configure leaking services of the core bundles as resettable.

in next PRs to come yes!

derrabus · 2017-09-12T08:49:31Z

About the config option: This feature should support PHPPM and other scenarios where we reuse the kernel after one request. I would consider those scenarios as rather exotic. The default case for php application is still a shared-nothing architecture and if I cannot disable this feature there, Symfony would consume CPU time to cleanup a container just to throw it away immediately afterwards.

nicolas-grekas · 2017-09-12T09:26:21Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+ */
+class ResettableServicePass implements CompilerPassInterface
+{
+    const RESET_TAG = 'kernel.reset';


should be removed, we don't use consts for tags

nicolas-grekas · 2017-09-12T09:26:46Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

@@ -117,6 +118,7 @@ public function build(ContainerBuilder $container)
        $container->addCompilerPass(new CachePoolPrunerPass(), PassConfig::TYPE_AFTER_REMOVING);
        $this->addCompilerPassIfExists($container, FormPass::class);
        $container->addCompilerPass(new WorkflowGuardListenerPass());
+        $container->addCompilerPass(new ResettableServicePass('kernel.reset'), PassConfig::TYPE_AFTER_REMOVING);


'kernel.reset' should be the default value of the constructor arg instead (as done in other passes)

nicolas-grekas · 2017-09-12T09:28:06Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+            $services[$id] = new Reference($id, ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE);
+            $methods[$id] = array();
+
+            foreach ($tags as $attributes) {


should be removed, no need for multiple methods, we should use only the one in $tags[0]['method']

nicolas-grekas · 2017-09-12T09:29:48Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+        }
+
+        $container->getDefinition(ServiceResetListener::class)
+            ->setArguments(array(


to allow for some flexibility, this should use replaceArgument (the xml declaration will need empty placeholders for pre-declaring those: <argument /> + <argument type="collection" />)

nicolas-grekas · 2017-09-12T09:30:23Z

src/Symfony/Component/HttpKernel/EventListener/ServiceResetListener.php

+class ServiceResetListener implements EventSubscriberInterface
+{
+    /**
+     * @var \Traversable


redundant with constructor declaration, should be removed

nicolas-grekas · 2017-09-12T09:30:28Z

src/Symfony/Component/HttpKernel/EventListener/ServiceResetListener.php

+    private $services;
+
+    /**
+     * @var array


nicolas-grekas · 2017-09-12T09:32:52Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+     */
+    public function process(ContainerBuilder $container)
+    {
+        $services = $methods = array();


if these are empty after processing, the listener should be removed, isn't it?

That's probably a good idea.

stof · 2017-09-12T09:43:09Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

@@ -117,6 +118,7 @@ public function build(ContainerBuilder $container)
        $container->addCompilerPass(new CachePoolPrunerPass(), PassConfig::TYPE_AFTER_REMOVING);
        $this->addCompilerPassIfExists($container, FormPass::class);
        $container->addCompilerPass(new WorkflowGuardListenerPass());
+        $container->addCompilerPass(new ResettableServicePass('kernel.reset'), PassConfig::TYPE_AFTER_REMOVING);


doing this after removing is a bad idea, as some resettable services may get inlined (due to being referenced only once before this pass adds a second reference). It should stay before optimizations are performed.
the general rule is that anything adding references should never run after optimizations (as optimizations are applied based on references). Removing references is fine (it may make us miss some optimization opportunities, but it cannot break the container)

This was a suggestion by @nicolas-grekas. I see your point and would revert this line, if it's okay with @nicolas-grekas.

I'm OK yes, let's revert

nicolas-grekas

last comments :)

nicolas-grekas · 2017-09-12T10:14:22Z

src/Symfony/Bundle/FrameworkBundle/FrameworkBundle.php

@@ -117,6 +118,7 @@ public function build(ContainerBuilder $container)
        $container->addCompilerPass(new CachePoolPrunerPass(), PassConfig::TYPE_AFTER_REMOVING);
        $this->addCompilerPassIfExists($container, FormPass::class);
        $container->addCompilerPass(new WorkflowGuardListenerPass());
+        $container->addCompilerPass(new ResettableServicePass(), PassConfig::TYPE_AFTER_REMOVING);


so, as discussed: let's remove TYPE_AFTER_REMOVING, my bad

nicolas-grekas · 2017-09-12T10:16:59Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+            $methods[$id] = $attributes['method'];
+        }
+
+        $container->getDefinition(ServiceResetListener::class)


The method should start with if (!$container->has(ServiceResetListener::class)) { return; }, and here we should use "findDefinition" instead of "getDefinition", but "removeDefinition" when $services is empty.

nicolas-grekas · 2017-09-12T10:17:47Z

src/Symfony/Component/HttpKernel/EventListener/ServiceResetListener.php

+class ServiceResetListener implements EventSubscriberInterface
+{
+    private $services;
+


derrabus · 2017-09-12T10:20:56Z

I hope, I've caught every comment. The PR looks mergable to me now. I've left my todo list in the PR description so we know the steps for follow-up PRs.

stof · 2017-09-12T10:24:22Z

The config option looks sensible to me, as we should indeed avoid resetting services when the process will die anyway.

stof · 2017-09-12T10:26:21Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+
+        foreach ($container->findTaggedServiceIds($this->tagName, true) as $id => $tags) {
+            $services[$id] = new Reference($id, ContainerInterface::IGNORE_ON_UNINITIALIZED_REFERENCE);
+            list($attributes) = $tags;


I think $attributes = $tags[0] would be more readable (and it is strictly equivalent, as list($attributes) expects the id to be 0, not anything)

nicolas-grekas · 2017-09-12T10:30:27Z

I'm personally not sure about another config option, because config options add complexity for users.
Here, this hook point is really nice for eg cache pools that could then clear their deferred item lists.
This happens at destruct time right now, but this is kinda a hack, destruct time to do biz logic is not the best.
We could instead document that the logic has to be as fast as possible (like emptying an array most of the time).

If this needs a longer discussion, I suggest doing it after this PR has been merged?

derrabus · 2017-09-12T12:41:01Z

If this needs a longer discussion, I suggest doing it after this PR has been merged?

I agree.

ogizanagi

👍

ogizanagi · 2017-09-14T15:59:42Z

src/Symfony/Component/HttpKernel/DependencyInjection/ResettableServicePass.php

+            $attributes = $tags[0];
+
+            if (!isset($attributes['method'])) {
+                throw new RuntimeException(sprintf('Tag %s requires the attribute method to be set.', $this->tagName));


Perhaps

- Tag %s requires the attribute method to be set. + Tag %s requires the "method" attribute to be set.

for clarity sake (and consistency with the similar exception messages in the core).

Yes, that's better. Changed.

ogizanagi · 2017-09-14T16:02:46Z

src/Symfony/Component/HttpKernel/Tests/EventListener/ServiceResetListenerTest.php

+class ServiceResetListenerTest extends TestCase
+{
+    /**
+     * @before


We never used this annotation in Symfony core (AFAIK). Shouldn't we use setUp as usual?

All right. Changed.

fabpot · 2017-09-14T18:14:19Z

@derrabus Can you make the 2 little changes asked by @ogizanagi so that I can merge this one? Thanks.

derrabus · 2017-09-14T20:26:35Z

@fabpot Done.

nicolas-grekas · 2017-09-14T20:26:53Z

src/Symfony/Component/HttpKernel/Tests/EventListener/ServiceResetListenerTest.php

+
+class ServiceResetListenerTest extends TestCase
+{
+    public function setUp()


should be protected

fabpot · 2017-09-14T20:37:22Z

Thank you @derrabus.

…e services (derrabus) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle][HttpKernel] Add DI tag for resettable services | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23984 | License | MIT | Doc PR | TODO This PR uses #24033 to introduce a DI tag for resettable services. TODO after merge: * Add an interface, make the "method" attribute optional and enable autoconfiguration. * Consider adding a config option to enable/disable this feature. * Configure leaking services of the core bundles as resettable. Commits ------- d9a6b76 A DI tag for resettable services.

…rrabus) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Reset stopwatch between requests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23984 | License | MIT | Doc PR | N/A Follow-up to #24155. This PR ensures that the stopwatch is reset between requests. Commits ------- 7c3bdd9 Reset stopwatch.

…n requests (derrabus) This PR was merged into the 3.4 branch. Discussion ---------- [SecurityBundle] Reset the authentication token between requests | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23984 | License | MIT | Doc PR | N/A Follow-up to #24155. This PR resets the token storage, making sure there's no user logged in at the beginning of a new request. Commits ------- e46b366 Reset the authentication token between requests.

… pool's local state (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [Cache] Add ResettableInterface to allow resetting any pool's local state | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - To allow pools to leverage #24155 so that they can be used in multi-request loops. Commits ------- 14c91f2 [Cache] Add ResettableInterface to allow resetting any pool's local state

This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle][HttpKernel] Reset profiler | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #18244 | License | MIT | Doc PR | N/A This PR adds the ability to reset the profiler between requests. Furthermore, the profiler service has been tagged with the new `kernel.reset` tag from #24155. For this, I had to readd the ability to define multiple reset methods for a service. Note: This PR requires twigphp/Twig#2560. Commits ------- 8c39bf7 Reset profiler.

carsonbot added Status: Needs Review FrameworkBundle Feature labels Sep 11, 2017

derrabus force-pushed the 3.4-resettable-services branch from 9847c51 to 2aad68b Compare September 11, 2017 14:42

derrabus mentioned this pull request Sep 11, 2017

[DX] Promote phppm and add support for resettable services #23984

Closed

stof reviewed Sep 11, 2017

View reviewed changes

nicolas-grekas reviewed Sep 11, 2017

View reviewed changes

nicolas-grekas added this to the 3.4 milestone Sep 11, 2017

nicolas-grekas changed the title ~~[FrameworkBundle][WIP] A DI tag for resettable services~~ [FrameworkBundle] A DI tag for resettable services Sep 12, 2017

nicolas-grekas changed the title ~~[FrameworkBundle] A DI tag for resettable services~~ [FrameworkBundle] Add DI tag for resettable services Sep 12, 2017

derrabus force-pushed the 3.4-resettable-services branch 2 times, most recently from ac7b2c6 to 9d2d4e8 Compare September 12, 2017 09:30

nicolas-grekas reviewed Sep 12, 2017

View reviewed changes

derrabus force-pushed the 3.4-resettable-services branch from 9d2d4e8 to f39ac55 Compare September 12, 2017 09:42

stof requested changes Sep 12, 2017

View reviewed changes

derrabus force-pushed the 3.4-resettable-services branch 4 times, most recently from 88b0983 to ea77c45 Compare September 12, 2017 10:14

nicolas-grekas reviewed Sep 12, 2017

View reviewed changes

nicolas-grekas changed the title ~~[FrameworkBundle] Add DI tag for resettable services~~ [FrameworkBundle][HttpKernel] Add DI tag for resettable services Sep 12, 2017

stof requested changes Sep 12, 2017

View reviewed changes

derrabus force-pushed the 3.4-resettable-services branch from ea77c45 to 4d05869 Compare September 12, 2017 12:40

ogizanagi approved these changes Sep 14, 2017

View reviewed changes

fabpot approved these changes Sep 14, 2017

View reviewed changes

derrabus force-pushed the 3.4-resettable-services branch from 6d3f424 to 8499bca Compare September 14, 2017 20:25

nicolas-grekas reviewed Sep 14, 2017

View reviewed changes

A DI tag for resettable services.

d9a6b76

derrabus force-pushed the 3.4-resettable-services branch from 8499bca to d9a6b76 Compare September 14, 2017 20:29

fabpot merged commit d9a6b76 into symfony:3.4 Sep 14, 2017

derrabus deleted the 3.4-resettable-services branch September 14, 2017 20:50

nicolas-grekas mentioned this pull request Sep 15, 2017

[Cache] Add ResettableInterface to allow resetting any pool's local state #24226

Merged

This was referenced Sep 22, 2017

[FrameworkBundle][HttpKernel] Reset profiler #24289

Merged

[SecurityBundle] Reset the authentication token between requests #24291

Merged

dmaicher mentioned this pull request Oct 13, 2017

[FrameworkBundle] make resettable services configurable/opt-in on 3.4? #24552

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

derrabus mentioned this pull request Oct 26, 2017

[HttpKernel][HttpFoundation] Prototype: Service reset as middleware #24697

Closed

Sander-Toonen mentioned this pull request Jan 26, 2018

Support PHP-PM by resetting state sampart/BreadcrumbsBundle#87

Closed

supersmile2009 mentioned this pull request Apr 18, 2018

Close connections on shutdown snc/SncRedisBundle#415

Merged

dkarlovi mentioned this pull request May 10, 2018

Connections not properly closed on Kernel shutdown snc/SncRedisBundle#400

Closed

nicolas-grekas mentioned this pull request Jun 22, 2018

Add symfony/contracts: a set of abstractions extracted out of the Symfony components #27093

Merged

dnna mentioned this pull request Aug 25, 2018

Add state resetting at the end of the request xyNNN/GoogleTagManagerBundle#31

Merged

nicolas-grekas mentioned this pull request Sep 9, 2018

Flush loggers on kernel.reset symfony/monolog-bundle#279

Merged

jdeniau mentioned this pull request Feb 20, 2019

Difference between parameter injection and access to getContainer Behat/Symfony2Extension#148

Closed

[FrameworkBundle][HttpKernel] Add DI tag for resettable services #24155

[FrameworkBundle][HttpKernel] Add DI tag for resettable services #24155

Conversation

derrabus commented Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Sep 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 11, 2017

derrabus commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas Sep 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derrabus commented Sep 12, 2017

stof commented Sep 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Sep 12, 2017 • edited Loading

derrabus commented Sep 12, 2017

ogizanagi left a comment

Choose a reason for hiding this comment

ogizanagi Sep 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 14, 2017

derrabus commented Sep 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Sep 14, 2017

derrabus commented Sep 11, 2017 •

edited

Loading

nicolas-grekas Sep 11, 2017 •

edited

Loading

derrabus Sep 12, 2017 •

edited

Loading

nicolas-grekas Sep 12, 2017 •

edited

Loading

nicolas-grekas commented Sep 12, 2017 •

edited

Loading

ogizanagi Sep 14, 2017 •

edited

Loading