[FrameworkBundle] add config option for resetting services #24554
Conversation
| @@ -107,6 +107,7 @@ public function getConfigTreeBuilder() | |||
| ->beforeNormalization()->ifString()->then(function ($v) { return array($v); })->end() | |||
| ->prototype('scalar')->end() | |||
| ->end() | |||
| ->booleanNode('reset_services')->defaultFalse()->end() | |||
There was a problem hiding this comment.
any better idea for the name?
There was a problem hiding this comment.
reset_services_on_terminate?
no more comment :)
|
@nicolas-grekas @derrabus @xabbuh any comments before I tackle the remaining todos? |
| @@ -317,6 +318,10 @@ public function load(array $configs, ContainerBuilder $container) | |||
| $loader->load('web_link.xml'); | |||
| } | |||
|
|
|||
| if (!$config['reset_services']) { | |||
There was a problem hiding this comment.
Instead of registering and removing the service, I'd rather invert that condition and only register the service if the feature has been enabled.
|
Should I work on the documentation PR? Any more comments here? How do we move forward? 😊 |
| @@ -317,6 +318,10 @@ public function load(array $configs, ContainerBuilder $container) | |||
| $loader->load('web_link.xml'); | |||
| } | |||
|
|
|||
| if ($config['reset_services_on_terminate']) { | |||
| $container->register(ServiceResetListener::class)->addTag('kernel.event_subscriber'); | |||
| @@ -74,11 +74,5 @@ | |||
| <service id="Symfony\Component\Config\Resource\SelfCheckingResourceChecker"> | |||
| <tag name="config_cache.resource_checker" priority="-990" /> | |||
| </service> | |||
|
|
|||
| <service id="Symfony\Component\HttpKernel\EventListener\ServiceResetListener"> | |||
There was a problem hiding this comment.
As discussed in #24634, we should keep that in xml, and remove it in the pass (sorry for the back and forth)
There was a problem hiding this comment.
Ok no problem 😉 Done.
|
I'm trying to figure out a way to not add any option (because any option makes Symfony harder to use). |
|
@nicolas-grekas interesting idea! Testing it now 😉 |
|
@nicolas-grekas my test problems are fixed! I tried this: class ServiceResetListener implements EventSubscriberInterface
{
private $services;
private $resetMethods;
private $calledOnMasterRequest = false;
public function __construct(\Traversable $services, array $resetMethods)
{
$this->services = $services;
$this->resetMethods = $resetMethods;
}
public function onKernelRequest(GetResponseEvent $e)
{
if (!$e->isMasterRequest()) {
return;
}
if ($this->calledOnMasterRequest) {
foreach ($this->services as $id => $service) {
$method = $this->resetMethods[$id];
$service->$method();
}
} else {
$this->calledOnMasterRequest = true;
}
}
/**
* {@inheritdoc}
*/
public static function getSubscribedEvents()
{
return array(
KernelEvents::REQUEST => array('onKernelRequest', 2048),
);
}
}However to simulate two master requests being handled this should work in theory in my $request = Request::createFromGlobals();
$kernel->handle($request);
$response = $kernel->handle($request);
$response->send();I'm getting a weird error: I'm guessing something is reset too early now maybe? |
|
@derrabus WDYT here? |
|
Ok so I understand the problem now. The So on the second master request the Catching the exception works fine: case KernelEvents::RESPONSE:
$token = $event->getResponse()->headers->get('X-Debug-Token');
try {
$this->stopwatch->stopSection($token);
} catch (\LogicException $e) {
}
break;This means that we cannot correctly profile the total master request time now for master requests Otherwise we need to reset the StopWatch maybe inside In general I like this new idea though. |
|
@nicolas-grekas If we want to get the service reset off the terminate event, I see two options. Either we do the reset as the first thing in |
|
@derrabus why not make that early request listener? |
|
Because we profile the event dispatcher. If we reset at the beginning of the request, we need to do it before we use the event dispatcher. |
|
@nicolas-grekas @derrabus actually also the current solution merged into 3.4 makes it impossible to profile the What if we have a Something like this: diff --git a/src/Symfony/Component/HttpKernel/Kernel.php b/src/Symfony/Component/HttpKernel/Kernel.php
index 6f4f2b7..8b2efa4 100644
--- a/src/Symfony/Component/HttpKernel/Kernel.php
+++ b/src/Symfony/Component/HttpKernel/Kernel.php
@@ -186,6 +186,10 @@ abstract class Kernel implements KernelInterface, RebootableInterface, Terminabl
$this->boot();
}
+ if ($type === HttpKernelInterface::MASTER_REQUEST && $handledMasterRequest) {
+ $this->container->get(ServiceResetter::class)->reset();
+ }
+
return $this->getHttpKernel()->handle($request, $type, $catch);
}Not sure about adding Or we do it inside |
@dmaicher Wrong. The profiler itself has always been triggered on the
Well yes, that's what I meant when I said:
This would lower the impact of the feature on existing Symfony projects and thus fix your problem. Downside is that we would move logic into a kernel that should be kept small and generic.
Well, call it |
|
Looking at the code, doing it in |
|
I don't think, Prototype: https://github.com/derrabus/symfony/tree/3.4-resetter-middleware |
|
See #24689 |
|
@derrabus I see no reason why it cannot be the responsibility of |
|
Well, so far, You're introducing an unexpected side-effect to something that's supposed to be a very simple data structure operation. I'd strongly advise against that. |
|
It's not unexpected: "you" passed the arguments to the constructor. Another way could be to decorate the original RequestStack. Not sure it's required, but if that's the only thing we can agree on, why not. That'd be still better than doing an HttpKernel middleware. |
Actually, it's the framework that does that. If I pull the request stack from the container, I'd expect a dumb data structure with requests in it.
Better, because that decorator could be part of the HttpKernel component, so we keep HttpFoundation clean. But the main problem remains for me.
What's so bad about that approach? The middleware in my prototype has 66 LOC, is self-contained and completely decoupled from the kernel. (I could even imagine that the profiler could benefit from running as a middleware, but that's a different story.) |
|
See #24697 |
As mentioned in #24552 the new resettable services feature has some BC breaks when it comes to functional tests and accessing services from the client's container after performing a request.
This PR makes the feature opt-in so it can be enabled if its needed.
TODOs: