Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[EventDispatcher] Fix eventListener wrapper loop in TraceableEventDispatcher#29376
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ro0NL commentedNov 30, 2018
👍 included this also in#29312 |
Uh oh!
There was an error while loading.Please reload this page.
| } | ||
| } | ||
| }finally { | ||
| $this->postDispatch($eventName,$event); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
in#29312 i actually hesitated if we should call postDispatch after exception. That's a new behavior, where the user cant act upon as it doesnt know about an errored dispatch call.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hm, looking at
symfony/src/Symfony/Component/HttpKernel/Debug/TraceableEventDispatcher.php
Lines 61 to 81 in5ba4997
| protectedfunctionpostDispatch($eventName,Event$event) | |
| { | |
| switch ($eventName) { | |
| case KernelEvents::CONTROLLER_ARGUMENTS: | |
| $this->stopwatch->start('controller','section'); | |
| break; | |
| case KernelEvents::RESPONSE: | |
| $token =$event->getResponse()->headers->get('X-Debug-Token'); | |
| $this->stopwatch->stopSection($token); | |
| break; | |
| case KernelEvents::TERMINATE: | |
| // In the special case described in the `preDispatch` method above, the `$token` section | |
| // does not exist, then closing it throws an exception which must be caught. | |
| $token =$event->getResponse()->headers->get('X-Debug-Token'); | |
| try { | |
| $this->stopwatch->stopSection($token); | |
| }catch (\LogicException$e) { | |
| } | |
| break; | |
| } | |
| } |
nicolas-grekas commentedDec 1, 2018
Thank you@jderusse. |
…bleEventDispatcher (jderusse)This PR was merged into the 3.4 branch.Discussion----------[EventDispatcher] Fix eventListener wrapper loop in TraceableEventDispatcher| Q | A| ------------- | ---| Branch? | 3.4| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none| License | MIT| Doc PR | naThe `TracableEventDispatcher` wrap decorate (in the method `preProcess`) each listeners in a `WrappedListener` before delegating the dispatch to the real dispatcher, then remove the wrapper (in the method `postProcess`.But, if a listener triggers an exception, the `postProcess` method is not called, and the wrapper in not removed.If the same event is triggered a second time, the listeners will be decorated twice, etc, etc..This is an issue with php-pm where the same event is triggered hundred of times within the same process.This PR moves the `postProcess` in a finally block in order to be called even if an exception in thrown.Commits-------3830a9e Fix wrapped loop of event listener
The
TracableEventDispatcherwrap decorate (in the methodpreProcess) each listeners in aWrappedListenerbefore delegating the dispatch to the real dispatcher, then remove the wrapper (in the methodpostProcess.But, if a listener triggers an exception, the
postProcessmethod is not called, and the wrapper in not removed.If the same event is triggered a second time, the listeners will be decorated twice, etc, etc..
This is an issue with php-pm where the same event is triggered hundred of times within the same process.
This PR moves the
postProcessin a finally block in order to be called even if an exception in thrown.