Sentry-php: Bug with multiple error handlers in specific contexts

[SunflowerFuchs]

Expected behaviour

All warnings are sent to sentry.

Actual behaviour

Only the first warning is sent to sentry, afterwards the sentry error handler gets unregistered.

Reproduction

First, the code has to be run on a webserver, otherwise the offending piece of code is never called.
I've set up a repo with the minimal requirements for reproducing this problem.
https://github.com/SunflowerFuchs/sentry-error

Offending piece of code

The actual unsetting of the error handler happens outside of sentry itself, in guzzlehttp/psr7. I will open an issue with them too, but I feel like there should at least be some tests here that should prevent issues like this in the future.

When sentrys errorhandler runs, at some point it tries getting the size of the request body.
https://github.com/getsentry/sentry-php/blob/7acdcdd1d507e29a38b4c282527a2ae3c9fca130/src/Integration/RequestIntegration.php#L181-L185
At which point guzzlehttp/psr7 takes over, and at some point tries setting, and afterwards unsetting, an errorhandler.
https://github.com/guzzle/psr7/blob/4ae8647dd962ac404d578fd42e00ffc81cb00d75/src/functions.php#L311-L321
But since this piece of code is already being run within an errorhandler, restoring the previous errorhandler destroys the current errorhandler from sentry.

---

It's possible that this is a WONTFIX since the main problem is possibly outside of sentry, but I'm submitting here mainly so tests can be improved, and in case anyone else possibly comes across this problem.

[ste93cry]

Unfortunately, this is a bug with PHP itself which has been reported in the past and that actually I don't think can be solved in any way in userland. The issue is that to prevent recursion PHP pops from the stack of handlers the current handler, saves it in a temporary variable and only then it calls it. At the end of the execution of the handler the value is pushed again into the stack. If someone else during the handling sets another handler, what happens is that the value in the temp variable is replaced, thus the reference to the original error handler is lost: to simplify, recursion is not handled well enough to account for this case. In HHVM instead it's fixed as they are handling this. A reference to a bug caused by the same issue is https://bugs.php.net/bug.php?id=63206

[not-implemented]

I currently ran into the same problem. In fact, when having a custom error handler defined before Sentry\init(), only the first error/warning/notice is logged via Sentry ... for the following ones, the first error-handler is called, which is really not the expected behaviour.

I also do not find a simple workaround inside sentry-php.

Maybe the error handling in try_fopen in guzzle/psr7 can be rewritten as a workaround. I'll create a pull-request for https://github.com/guzzle/psr7/issues/318 there. But yes, the original problem is in the PHP core with the error-handler-stack.

[not-implemented]

PHP Bug https://bugs.php.net/bug.php?id=63206 should be fixed now. The pull-request (https://github.com/php/php-src/pull/5206) has already been accepted. So the fix should be in the next PHP 7.3 and 7.4 patch releases.

By the way: The workaround in https://github.com/getsentry/sentry-php/blob/master/src/ErrorHandler.php#L176 should also be obsolete then :-)

[Jean85]

Unfortunately it's not, since we support PHP 7.1 and 7.2 too.

[not-implemented]

@Jean85: Ok ... at least in the long-term future ;-)

[Jean85]

Well, we could check the PHP version to limit the workaround, but I don't know if it makes sense.

BTW thanks a lot for that patch! I hadn't any hope that that bug would've been fixed!

[not-implemented]

At least, it is fixed at the right place now.

But ... damn it, I see a lot more issues now in our production system than before ... I didn't want to know this! 😋 😀 😀 ...