Reset of error handler in Promise\fatalError

[ghost]

Is there any reason why the internal function fatalError resets the error handler, even though the library does not even set one in the first place? A nasty side effect of this is that it removes an user defined error handler - in my case would that be a throwing error handler, leading to regular PHP errors, which can't be handled through try-catch.

promise/src/functions.php

Lines 209 to 220 in c1aad8e

	function fatalError($error)
	{
	try {
	trigger_error($error, E_USER_ERROR);
	} catch (\Throwable $e) {
	set_error_handler(null);
	trigger_error($error, E_USER_ERROR);
	} catch (\Exception $e) {
	set_error_handler(null);
	trigger_error($error, E_USER_ERROR);
	}
	}

The relevant commit for this does not mention why a reset is necessary.

[jsor]

The intention is, that errors ending here are not being handled properly before (eg. and exception thrown in the $onRejected callback registered via done()) and thus must abort the program. This is a programming error, similar to an uncaught exception.

[ghost]

That is the intention of the whole function, yes. But I dont really get why there is a try catch block. If I throw an exception from my error handler, a library shouldn't catch it, unset my error handler and trigger it as regular error again. The intention of trying to abort the program and overwrite an user defined behaviour of what happens when a PHP error gets triggered shouldn't be modified to get the intended result. Exactly because done does not catch an exception I could use this behaviour to fallback to some sort of top level catch block to apply custom logic. However due to the behaviour of trying to catch an exception from triggering an error, the user defined error handler gets overwritten. In the end if the error handler just does something else than triggering an exception (like just log the error and good), your intentions are never executed. In my opinion the try catch block should be removed, exactly because you can never exactly fulfill it and because you overwrite an user defined behaviour.

[jsor]

The second argument of done() is your top level catch block. If there is an exception thrown in that block, it is equivalent if code in the top level catch block of an synchronous application throws (which results in an fatal error for an uncaught exception).

In an promise-based asynchronous application, you should avoid try-catch blocks because you most likely also use an event loop and the code throwing might be in a different call stack than the try-catch block. The promise API mimics exactly the try-catch-finally blocks from synchronous applications.

try {
    $response = app();
    echo $response;
} catch (\Throwable $e) {
    logUnhandledException($e);
    echo "Unhandled fatal error $e";
} finally {
   echo "Thank you for flying with us!";
}

is the synchronous equivalent to the following async one:

app()
    ->then(function ($response) {
        echo $response;
    })
    ->otherwise(function (\Throwable $e) {
        logUnhandledException($e);
        echo "Unhandled fatal error $e";
    })
    ->always(function () {
       echo "Thank you for flying with us!";
    })
    ->done()
;

(The above code also demostrates my personal preference to explicitely "catch" exceptions with otherwise() and just end the promise chain with done())

In both cases, if logUnhandledException() throws, it will result in an fatal error. done() here is only a mechanism to not swallow the exception thrown inside the otherwise()callback.

[jsor]

Since you closed this issue, i just wanted to ask if i explained the intention of this behaviour good enough? I'm still interested in your use case. It is still prefectly possible that we miss something.

[ghost]

You explained it. I understood it, but I don't think that a library should ever mess with error handlers. It's not something that a library should care about. It's an application's responsibility to handle this.

I'm not per se against making it a "pseudo" fatal error when the done handler throws an exception. I'm against messing with error handler because it works entirely against an application's philosophy.

But I don't think that my sole opinion for this philosophy would change anything, so I decided to not further persue it.

[jsor]

I definitely see you point and we probably can do better here. The intention of the change was, to not let any exceptions bubble out of the callbacks because it's handling is then unpredictable. Depending on the situation, it might end up in the recjection chain, bubble out etc. See also #88 which led to this change. That's the reason we do not allow the error handler to throw here.

An alternative approach could be to wrap the exception in something like an FatalErrorException so the error handler can detect it and act accordingly, eg. just log just this exception and rethrow all other errors as exceptions.

[ghost]

The decision to convert an error to an exception was however an application decision. If that now leads to exceptions bubbling up to reject or resolve callbacks, then that's unfortunate but shouldn't lead to library workarounds and explicit overrides. The exception stack trace shows where it was initially thrown (in the error handler with the original error) and thus leads to (hopefully) appropriate application developer actions.

I see why that change was necessary and I do think that something should be done, but not at the expense of explicitely working against an application. My error handler explicitely converts an error to an exception and if that now goes through all blocks and leads to an uncaught exception, then the Sentry client I've integrated would report this. However an unset error handler can't inform anyone of an error, because Sentry was never informed about it. The handler was explicitely unset to allow slipping through. Resulting to unreported bugs, application crash and downtime. Headache for every developer involved in the application.

The current behaviour isn't even to my knowledge documented, leading to even more headache because no one knows why the application error handler wasn't triggered (correctly).

I'm totally for making an exception class and letting that explicitely through all promise chains (if that's feasible) - or if not possible, let the user decide what action to take. If it's still a thrown exception, then the user has to deal with the consequences.

But just unsetting an application error handler makes no one happy.

[clue]

@CharlotteDunois Thanks for raising this issue, I agree that this looks weird. Can you gist the problem you're seeing, so we can take a look at how this could potentially be improved? 👍