inpsyde / Wonolog

Monolog-based logging package for WordPress.
https://inpsyde.github.io/Wonolog/
MIT License
172 stars 17 forks source link

[Bug]: Infinite recursion with unhandled exception due to restore_exception_handler #81

Open gaambo opened 1 month ago

gaambo commented 1 month ago

Description of the bug

I'm running into an infinite recursion when throwing an exception somehwere. The PhpErrorController:on_exception exception handler is correctly called and logs the exception, but it then calls restore_exception_handler and re-throws the Exception which triggers the same function again. According to a comment in the PHP docs:

Note that this does not work within an exception handler.

Therefore it runs the same method over and over again.

Reproduction instructions

  1. Bootstrap the default configuration
  2. Throw an exception somewhere in the code (e.g. on init)
  3. See how the request times out because of maximul call stack size and the log is full of the same error message
  4. alternative to 3: step-debug with xdebug and see the on_exception method get called over and over again

Expected behavior

In my case I just wanted to test the logging by manually throwing an exception. But when a real one throws, it loggs a huge amount of the same message and runs into fatal error with "Maximum call stack size".

I found out that you can use set_exception_handler(null); to reset it to the PHP defaults exception handler. Then the on_fatal will be called, because the exception is unhandled.

Another possibility would be to save the return-value of set_exception_handler (= the previous one) in Controller::log_php_errors and restore to it later.

Environment info

Relevant log output

Fatal error: Uncaught Error: Maximum call stack size of 8339456 bytes (zend.max_allowed_stack_size - zend.reserved_stack_size) reached. Infinite recursion? in /var/www/html/vendor/inpsyde/wonolog/src/Data/Log.php:222 Stack trace: #0 /var/www/html/vendor/inpsyde/wonolog/src/Data/HookLogFactory.php(104): Inpsyde\Wonolog\Data\Log->level() #1 /var/www/html/vendor/inpsyde/wonolog/src/Data/HookLogFactory.php(88): Inpsyde\Wonolog\Data\HookLogFactory->maybe_raise_level(0, Object(Inpsyde\Wonolog\Data\Log)) #2 /var/www/html/vendor/inpsyde/wonolog/src/Data/HookLogFactory.php(38): Inpsyde\Wonolog\Data\HookLogFactory->extract_log_objects_in_args(Array, 0) #3 /var/www/html/vendor/inpsyde/wonolog/src/LogActionSubscriber.php(67): Inpsyde\Wonolog\Data\HookLogFactory->logs_from_hook_arguments(Array, 0) #4 /var/www/html/web/wp/wp-includes/class-wp-hook.php(324): Inpsyde\Wonolog\LogActionSubscriber->listen(Object(Inpsyde\Wonolog\Data\Log)) #5 /var/www/html/web/wp/wp-includes/class-wp-hook.php(348): WP_Hook->apply_filters('', Array) #6 /var/www/html/web/wp/wp-includes/plugin.php(517): WP_Hook->do_action(Array) #7 /var/www/html/vendor/inpsyde/wonolog/src/PhpErrorController.php(116): do_action('wonolog.log', Object(Inpsyde\Wonolog\Data\Log)) #8 [internal function]: Inpsyde\Wonolog\PhpErrorController->on_exception(Object(Exception)) #9 {main} thrown in /var/www/html/vendor/inpsyde/wonolog/src/Data/Log.php on line 222

Additional context

No response

Code of Conduct

gaambo commented 1 month ago

I've got a simple workaround running, by extending PhpErrorController: https://gist.github.com/gaambo/6ea135cdc28fe3b30e61c2713db78040

I still need to test this further, I'm just getting started with Wonolog.

But if this works and it's in your interest, I can draft up a PR.

gmazzap commented 1 month ago

Thanks @gaambo

It never worked restoring the previous handler, but before PHP 8.3, it did not trigger an infinite loop either.

In the comment in the official docs you linked, where the exception is re-thrown, there is no mention of an endless loop because that is a new thing.

This is a bug, but it is only visible on PHP 8.3, which we are yet testing for...

If you can do a PR with the fix, I'm definitively interested, but I would like to avoid the requirement that Php_Error_Controller::register() must be called before bootstrap () for it to work.

gmazzap commented 1 month ago

Moreover, it looks like you're using Wonolog v1. You should look into v2 for more modern PHP support, or even in the upcoming v3

gmazzap commented 1 month ago

It might be something like this: https://3v4l.org/jme3m

The Configurator::setupPhpErrorListener() method in that 3v4l above would be here in Wonolog: https://github.com/inpsyde/Wonolog/blob/3.x/src/Configurator.php#L1013

gaambo commented 1 month ago

It never worked restoring the previous handler, but before PHP 8.3, it did not trigger an infinite loop either.

Very interesting finding, thank you @gmazzap.

If you can do a PR with the fix, I'm definitively interested, but I would like to avoid the requirement that Php_Error_Controller::register() must be called before bootstrap () for it to work.

That is just for my custom implementation for Php_Error_Controller, because the setup happens in the constructor. If it's in the framework, that's not required. I also just wanted to share the workaround for anyone coming here.

Moreover, it looks like you're using Wonolog v1. You should look into v2 for more modern PHP support, or even in the upcoming v3

I wasn't sure about their stability and therefore stuck to v1. But we'll probably want to use a 3rd party handler as well that requires at least monolog v2, so we'll probably update soon.

Should I create a PR for v1, v2 or v3? 😬