[EventDispatcher] Fix eventListener wrapper loop in TraceableEventDispatcher #29376
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
👍 included this also in #29312 |
ro0NL
reviewed
Nov 30, 2018
src/Symfony/Component/EventDispatcher/Debug/TraceableEventDispatcher.php
Show resolved
Hide resolved
d70a121 to
a137088
Compare
a137088 to
3830a9e
Compare
ro0NL
approved these changes
Nov 30, 2018
nicolas-grekas
approved these changes
Nov 30, 2018
ro0NL
reviewed
Dec 1, 2018
| } | ||
| } | ||
| } finally { | ||
| $this->postDispatch($eventName, $event); |
There was a problem hiding this comment.
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.
Hm, looking at
it should be OK and expected.|
Thank you @jderusse. |
nicolas-grekas
added a commit
that referenced
this pull request
Dec 1, 2018
…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 | na The `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
This was referenced Dec 6, 2018
Merged
Closed
Merged
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.