Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Infinite loop when handling an exception (critical bug in my opinion) #774

Closed
enumag opened this issue Feb 21, 2019 · 4 comments · Fixed by #776
Closed

Infinite loop when handling an exception (critical bug in my opinion) #774

enumag opened this issue Feb 21, 2019 · 4 comments · Fixed by #776

Comments

@enumag
Copy link
Contributor

enumag commented Feb 21, 2019

I'm using the Symfony bundle. I start my consumer using bin/console enqueue/consume and sure enough it works just fine.

However if one of the consumed messages causes an exception, the consumer freezes without even writing anything to console or log - the exception is nowhere to be seen.

I used xdebug to find out what caused it and I believe the reason is that Symfony/Console runs into an infinite loop while trying to print the exception.

The reason why that happens is this code in your bundle:

$wrapper = $e;
while ($prev = $wrapper->getPrevious()) {
if ($exception === $wrapper = $prev) {
throw $e;
}
}
if ($exception !== $wrapper) {
$prev = new \ReflectionProperty('Exception', 'previous');
$prev->setAccessible(true);
$prev->setValue($wrapper, $exception);
}

It effectively creates a pair of exceptions where each has set the other one as the previous:

$e->getPrevious()->getPrevious() === $e // true

(or a longer chain of exceptions where the last one has the first one as previous - still effectively a cycle of exceptions)

I don't quite understand what is the idea behind this but obviously when an error handler tries to dump the exception chain this causes an infinite loop because ->getPrevious() never returns null.

Can you explain what the code in QueueConsumer::onProcessorException() is supposed to do? I'm quite sure this isn't intentional. When I remove the code I linked above, everything works as expected.

@enumag
Copy link
Contributor Author

enumag commented Feb 21, 2019

Ping @nicolas-grekas. You're the original author of this code in Symfony. What on earth is the reason to create an infinite loop of exceptions?

@makasim
Copy link
Member

makasim commented Feb 21, 2019

As far as I understand this #725 fixes the issue. Could you check?

@enumag
Copy link
Contributor Author

enumag commented Feb 21, 2019

@makasim No it doesn't. I'm using v0.9.7 which already has this fix.

Maybe the condition there is supposed to be if ($exception !== $e) {? Because that's what happened in my case - $exception and $e are one and the same ($exception was rethrown here and then caught as $e) while $wrapper is different.

Also ping @DamienHarper - as the author of #725 you might have an idea.

@DamienHarper
Copy link
Contributor

@enumag Sorry I couldn't reply earlier, #725 helped me fix some issues I was experiencing at that time. Recently, I faced again some infinite loops, caused by those self-referencing exceptions, making some of my commands crash again. Though I hadn't enough time to investigate more.
So thanks for the fix.

nicolas-grekas added a commit to symfony/symfony that referenced this issue Feb 22, 2019
…mag)

This PR was merged into the 3.4 branch.

Discussion
----------

[HttpKernel] Fix possible infinite loop of exceptions

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

I ran into an [issue](php-enqueue/enqueue-dev#774) in the enqueue library which copied this part of code from Symfony. I'm now starting to understand what the problem is and it should most likely be fixed in Symfony as well.

I didn't actually run into it in Symfony itself but it seems at least hypothetically possible. Imagine if [here](https://github.com/symfony/symfony/blob/8c3dc8254a508593aa0637445659e93e39d31dca/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php#L77) `$e` is somehow the same (===) as `$exception`. The code [below](https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php#L82-L92) will then find the last exception in the `getPrevious()` chain and assigns `$exception` as the previous. However in the off chance that `$exception` is actually `$e` (the first exception in the chain) then it creates an infinite loop of exceptions which is not good for monolog and exception handlers.

What do you think?

Commits
-------

3447222 [HttpKernel] Fix possible infinite loop of exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants