[HttpFoundation][HttpKernel] Move ServiceResetListener logic to RequestStack #24689
Conversation
02f5f00 to
e1b5839
Compare
|
Doing this in the RequestStack looks weird to me. I don't think it belongs to the RequestStack. It would be more logic to have the Kernel handling it (near the time it pushes a request to the stack). |
|
I agree with @stof ... this logic looks out of place inside RequestStack. Sadly I can't provide a specific alternative about where to put this. |
|
Doing so in the |
|
Note that performance-wise, the current code will always be the fastest. |
e1b5839 to
972297a
Compare
| @@ -18,7 +18,7 @@ | ||
| "require": { | ||
| "php": "^5.5.9|>=7.0.8", | ||
| "symfony/event-dispatcher": "~2.8|~3.0|~4.0", | ||
| "symfony/http-foundation": "^3.3.11|~4.0", | ||
| "symfony/http-foundation": "^3.4|~4.0", |
There was a problem hiding this comment.
transitively, this is already the lowest version
|
Now refactored as an inheritance proxy so that only |
|
|
||
| return; | ||
| if (!empty($services)) { | ||
| $container->register('request_stack', ResettingRequestStack::class) |
There was a problem hiding this comment.
Shouldn't we decorate the existing request stack rather than replacing it?
There was a problem hiding this comment.
Someone might already have replaced the request stack by his own, or decorated it for his own needs.
There was a problem hiding this comment.
OK, indeed. So I'm going to add a runtime decorating proxy.
There was a problem hiding this comment.
Wait no! that's not how decoration works! So there is no issue with it here.
We don't guarantee BC for ppl that just replace services (otherwise we would be in serious trouble moving forward).
So as is is all good, isn't it?
There was a problem hiding this comment.
A request_stack decorator would just end up with a ResettingRequestStack instance as inner since this pass runs before decoration happens. Seems fine
| private $resettableServices; | ||
| private $resetMethods; | ||
|
|
||
| public function __construct(\Traversable $resettableServices, array $resetMethods) |
There was a problem hiding this comment.
previously the services were instantiated lazily together with the listener on dispatch of kernel.terminate, right? Seems now the services would be instantiated before the first master request is handled already, even though they are not necessarily all used during the request.
Means now its affecting performance maybe a bit more than before? I mean before at least it was done after the response was streamed back to the client. Now it will delay handling the first request in some cases maybe.
Maybe its possible to do it completely lazy only if they are actually reset (in case there is a second master request)?
There was a problem hiding this comment.
The iterator yields only instantiated services, so all is fine.
| * | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class ResettingRequestStack extends RequestStack |
There was a problem hiding this comment.
Could add an interface on the RequestStack and make this extension deprecated in 4.1 to promote composition over inheritance. Should probably also get @final
|
Just to document my concern I've raised in #24554 already: I don't think that |
|
@derrabus this is a decorator, not the main stack anymore. A decorator is the typical design pattern to add feature on top of a more basic one. Anyway, please provide an alternative solution, blocking is not enough if we want a fixed beta2. @iltar we don't need the interface, the inheritance decorator is really fine IMHO. |
|
To anyone who voted 👍 or 👎 here, please provide an actual solution instead. |
|
@nicolas-grekas At the moment this is already an extension point, so composition is not properly possible as they would have to extend the new class instead. I'm not using this feature so it doesn't concern me, nor would I touch the request stack in a way where I'd want to modify it by extending. However, it should be considered that others do, as it's public. |
|
@nicolas-grekas I did not mean to block anything, I just disagree with your solution. After all, I believe you chose it, not because you found I've submitted my alternative suggestion as #24697 now. |
As spotted in linked issues, resetting services should happen before running a 2nd request, and not after each one. This fixes collecting late data after
kernel.terminate, and frees the CPU from running reset methods when not using the kernel in a loop.This must happen before the "kernel.request" event is fired so that the event dispatcher can be profiled.
There is no better place than doing so in
RequestStack.