Our plans for unhandled rejected promises in Promise v3

[WyriHaximus]

For Promise v3 we want to handle unhandled rejected promises better. So we've dropped the done method, and are going to throw when we detect a rejected promise hasn't been handled. My initial implementation in reactphp/promise#170 would have harshly stopped the execution of the program. We decided to not do that because it would be unrecoverable. My current in reactphp/promise#222 implementation throws a rejected promise detects it is the last link in the chain. All of this has been brought up in reactphp/promise#87 in the past, and it's our final feature to check off for the v3 release.

When v3 lands with this feature in it, regardless of which form, it will be a big breacking change. The goal is to uncover all unhandled rejected promises, and make sure they are handled. To illestrate, in the following situation the rejected promise would go unnoticed:

resolve(true)->then(static fn() => throw new \Exception('Boom!'))->then();

However when you would add done at the end of the chain it would throw.

resolve(true)->then(static fn() => throw new \Exception('Boom!'))->then()->done();

With the changes we want to make the former example would behave as the latter without the additional method call. Which has the potential to uncover unexpected rejections and throw them where v2 didn't. This is why we're looking for our community feedback on this change. Because of it's potential huge impact. When this lands, well when reactphp/promise#170 specifically lands, a unhandled rejection will be thrown when it is no longer referenced. Which means that if you keep the reference longer around than needed it might blow up at a later unexpected point.

When, for whatever reason, you cannot update to Promise v3 when it lands. You can opt-out of it using the conflict feature of composer. Add this to your composer.json:

    "conflict": {
        "react/promise": "<3.0.0"
    },

[lucasnetau]

Is it possible to add a similar code path to a new release of the v2 branch and instead of throwing the Exception you trigger_error a E_USER_WARNING and don't die. This would allow for v2 users to see code that may blow up with the v3 release, easing the migration path.

[SimonFrings]

Hey @lucasnetau, good input 👍

I had a call with @clue and @WyriHaximus today, discussing our progress for "unhandled rejections". @clue is currently preparing a pull request for this which will go live in the next couple of days. In his approach all these unhandled rejections will be logged as error messages without throwing an Exception and killing the program execution, which sounds similar to what you're suggesting.

At first we considered warnings as well, but if any users convert warnings to exceptions, we'd have the same problem. It's always important for us to make an upgrade path as smooth as possible, so logging these messages instead of throwing an exception will be the better way to go about this.

[WyriHaximus]

To add to @SimonFrings message, outputting errors for this in v2 would potentially cause errors across our users projects in a minor release. And given the big impact that this change can have, having it in a new major release signals the massive nature of the change. Fully agree that an easier migration path is important, but not sure if that is wise in this specific use case.

[SimonFrings]

Update: @clue just filed a pull request which adds new functionality to make sure to report any unhandled promise rejections: reactphp/promise#248 🎉

The way it works is also described in this PR, so here's a quote:

Any unhandled promise rejections will currently always be written to PHP's default error log location (STDERR for cli, Apache error log, etc.). Once this PR is merged, I've prepared a follow-up PR to provide a custom log handler function to write to custom log locations similar to how set_exception_handler() and set_error_handler() work.

The next step is to update all ReactPHP components to handle any unhandled rejections. In most components this only affects the test suite, except for reactphp/promise-stream which also need some functional adjustments.

[lucasnetau]

Should a library be writing to STDERR via error_log() by default? That to me is considered incorrect behaviour (the same as a library polluting output with echo / print_r / var_dump. What if you have a wrapper around STDERR (eg monolog) and expect all output to be formatted?

trigger_error with a warning is more inline with how PHP raises notices/warnings/deprecations and at least gives the end user a chance to capture the warning and do something about it (eg log it how their app is doing logs). If a user is capturing warnings and upgrading them to exceptions then they have made that conscious decision in their codebase and would expect to be able to be alerted to unhandled rejections in the same way. There are many ways that PHP will raise a notice/warning so if they are upgrading to errors you aren't doing it by mistake.

[WyriHaximus]

FYI there should come a follow up PR soon to introduce the possibility to overwrite this behavior with a user defined way of handling it.

[SimonFrings]

What @WyriHaximus said 👆

@lucasnetau Good input and I can see your point! We understand that there are numerous projects out there, each with its own unique features and introducing changes that please everyone is nearly impossible. Therefore, we decided to prioritize the error log implementation as it would have the least impact on existing code bases, without disrupting program execution.

There's already a second pull request live which gives you more control over how unhandled rejections are being reported and by default, our error logging will still apply. However, if you want to make any changes, you can adjust it according to your needs once the PR is merged: reactphp/promise#249