Add PromiseErrorHandler to decouple the specification

[kelunik]

Removes the need for an event loop and decouples the two specifications.

It can be discussed whether there should be another static method PromiseErrorHandler::reset() that just removes all callbacks.

Resolves #18. Relates to #17, async-interop/event-loop#113.

[joshdifabio]

I feel like the following interface might be better since it's idempotent and would prevent people from having to hold onto handler ids in order to remove handlers later:

final class PromiseErrorHandler
{
    public static function setHandler($id, callable $handler);

    public static function removeHandler($id);
}

Unless I'm mistaken, removing the dependency on Loop means that some (or all?) promise libraries will be calling promise callbacks synchronously when promises are resolved. Is everyone happy with this? People don't think it's important to guarantee that callbacks are called asynchronously?

[kelunik]

@joshdifabio With that API you need unique IDs.

No, not at all. An implementation can choose to work only when a Loop is present. But then only that specific implementation depends on the Loop, not the specification itself.

Why do you think it's important to guarantee that callbacks are always called asynchronously?

[joshdifabio]

@joshdifabio With that API you need unique IDs.

Sure, but that's pretty easy. They don't need to be ints. PromiseErrorHandler::setErrorHandler('amp_error_handler', $fn);
PromiseErrorHandler::removeErrorHandler('amp_error_handler');

No, not at all. An implementation can choose to work only when a loop is present. But then only that specific implementation depends on the Loop, not the specification itself.

But it permits synchronous promise resolution, right? Is everyone happy with that?

[kelunik]

Sure, but that's pretty easy.

Until someone chooses the same rather generic identifier. I guess most handlers won't clean up anyway, so a ::reset() method would be good as stated previously.

But it permits synchronous promise resolution, right? Is everyone happy with that?

What's the problem you see? We already do that in Amp for already resolved promises. This is also not changed by this PR.

[bwoebi]

@joshdifabio Where does having fixed identifiers provide a benefit? It just adds global state to something dynamic (dynamic as in there can be multiple handlers), I'd say?

@kelunik Does any default behavior make sense? Maybe an E_USER_ERROR (yeah, a real fatal this time) that Promises are used with no error handler active?

Also @kelunik can we rename it to just "remove" and "add"? Handler is redundant in method name

[joshdifabio]

Until someone chooses the same rather generic identifier. I guess most handlers won't clean up anyway, so a ::reset() method would be good as stated previously.

Generally I think it's best if methods which change state don't return any value. In this case that's easily achievable which is why I made the suggestion, but I don't think it's important so I won't bang on about it!

What's the problem you see? We already do that in Amp for already resolved promises. This is also not changed by this PR.

I don't think it's necessarily a problem but I'm curious whether everyone is happy with allowing synchronous promise resolution. It's not exactly the norm in the JS world afaik. See an old ReactPHP discussion on the topic: reactphp/promise#4

[joshdifabio]

@joshdifabio Where does having fixed identifiers provide a benefit? It just adds global state to something dynamic (dynamic as in there can be multiple handlers), I'd say?

I'd say the opposite. With handlers identified by auto-generated ID's the ID has to be stored in some kind of global object in order for the handler to be removed later. With a manually specified ID this isn't necessary as a simple constant can be used to set and later remove the handler.

I tend to believe that the best functions are either pure side-effect-free functions, commands (i.e. they change state but return nothing), or queries. (Note that this isn't original thinking on my part.) Sometimes it's necessary to compromise on this but often it's not.

Still, I don't think this is very important.

[bwoebi]

@joshdifabio I pretty much see:

$id = PromiseErrorHandler::add($handler);
Loop::execute(...);
PromiseErrorHandler::remove($id);

Not sure where we'd have to store it in any global state here.

[joshdifabio]

I don't think this is worth the time we're spending discussing it. Go with what you think is best.

[trowski]

I agree with @bwoebi, triggering a E_USER_ERROR makes sense if no error handler is set. Also triggering an error may be a wise if a handler throws an exception?

I also agree that handler is redundant in the method names. add and remove seems sufficient.

[WyriHaximus]

It looks good @kelunik nice work 👍 But I agree with @trowski and @bwoebi we need to let errors bubble up. Since this is a last resort handler we can safely trigger an E_USER_ERROR when no handler is set or when the handler throws an error internally. And let the user clean up, tbh no error should ever be ignored or hidden and always be handled.

[kelunik]

Yes, I thought about doing an exit; there, but didn't think about trigger_error.

[kelunik]

@bwoebi @WyriHaximus I guess it should be E_RECOVERABLE_ERROR? If we have an error handler installed that handles it, it's actually fine.

...just that we can only throw E_USER_ errors. Not sure if E_USER_WARNING or E_USER_ERROR then.

[WyriHaximus]

@kelunik E_RECOVERABLE_ERROR sounds good to me but we can't throw that so E_USER_ERROR has my preference.

P.S. My two cents about exit;: it is generally a really bad idea to do that packages as it is influencing application flow.

[kelunik]

I don't think it's necessarily a problem but I'm curious whether everyone is happy with allowing synchronous promise resolution. It's not exactly the norm in the JS world afaik. See an old ReactPHP discussion on the topic: reactphp/promise#4

@joshdifabio Could you open another issue for that one?

[kelunik]

@WyriHaximus trigger_error(..., E_USER_ERROR); is basically a log(..); exit;.

[WyriHaximus]

@kelunik it is, but imho it is the application using this pakackage responsibility to handle that log and the exit not ours.

[kelunik]

Sure, we just add an emergency handler if nothing else is registered. Note that the error advising you to register at least one handler only appears if you had an error somewhere yet.

[WyriHaximus]

You're latest changes are perfect 👍 .

[kelunik]

I moved the error handler into the Interop\Async\Promise namespace and renamed it to ErrorHandler, so we don't pollute the Interop\Async namespace. I also added some tests and fixed a bug (__METHOD__ already includes the class name).

[bwoebi]

If you prefix something with "Promise", it very much is not pollution - but I'm also fine with it being in a Promise namespace instead.

[joshdifabio]

I don't think it's necessarily a problem but I'm curious whether everyone is happy with allowing synchronous promise resolution. It's not exactly the norm in the JS world afaik. See an old ReactPHP discussion on the topic: reactphp/promise#4

@joshdifabio Could you open another issue for that one?

Done. See #20

[bwoebi]

I've been thinking about this, I'm not sure about being able to install multiple handlers.

I'm thinking we should rather scope our Promise error handlers like we are scoping Loop::execute().

As you typically want your handler to be invoked (e.g. log errors inside your nested event loop to somewhere else) and not the original handler. We have a reset() method maybe, but that's only fine when we aren't planning to resume the original loop afterwards.

Or do you have any concrete reason why we need multiple handlers; ultimately the event loop also only has a single handler.

[kelunik]

@bwoebi For the event loop, multiple handlers don't make sense, as those handlers are either rethrow or be done with it. This handler is for pure notification, it doesn't matter how many callbacks are notified. We might have a default handler, that just logs these exceptions, but there might be another handler during bootstrap that makes startup fail entirely if there's already such an error in the bootstrap process. That said, scoping might be fine as well.

[bwoebi]

Isn't it the same here?

Quoting from Loop::setErrorHandler:

 * The callback receives the error as the first and only parameter. The return value of the callback gets ignored.
 * If it can't handle the error, it MUST throw the error. Errors thrown by the callback or during its invocation
 * MUST be thrown into the `run` loop and stop the driver.

If it doesn't rethrow, the loop just continues.

Same here:

if (empty($callbacks)) { trigger_error(…); }
foreach ($callbacks as $cb) {
    try { $cb($error); } catch (…) { trigger_error(…); }
}

If we don't rethrow, we're also done with it.

What do I misunderstand here?

---

If you want that another handler in bootstrap process, scoping will totally satisfy this. No need for multiple handlers there. If you say it might be fine as well, then please go with that.

[kelunik]

What do I misunderstand here?

A handler here explicitly MUST NOT throw, while it's just fine in a loop, because an exception there just bubbles up to Loop::execute().

[bwoebi]

Uh, I wouldn't say that. It can deliberately throw, knowing that it will lead to a program abortion.

Ultimately, exceptions bubbling up to Loop::execute() usually lead to the program being aborted too...

[kelunik]

Uh, I wouldn't say that. It can deliberately throw, knowing that it will lead to a program abortion.

No, they shouldn't. They should log the error themselves and exit cleanly with a proper exit code.

[bwoebi]

It makes no difference in the end result, the program is aborted anyways.

[kelunik]

@bwoebi After giving it some thought, scoping doesn't really make sense here. At least stacking those scopes does not. As we're using cooperative multitasking here, there will be other promises resolved from an "outer" scope if you try to "scope" your handler in a specific place.

But I guess we could remove the handlers and only allow one single handler at a time, which is set by the application.

[bwoebi]

Makes sense. Thus the API would be public function set(?callable $handler): ?callable (sets the new handler, returns the old one)?

[kelunik]

Any reason to return the previous one?

[bwoebi]

@kelunik to be able to reset it to the old one afterwards, e.g. if you opened a new loop context.

[joshdifabio]

Is it fair to say that the vast majority of promise consumers won't need to use this error handler? (Setting the error handler will probably be a once-per-application thing. Most of the time we just want to call when().)

With that in mind, would it make sense to create a separate package which contains the error handler and any other code which is only needed by promise implementors and application bootstrapping code? I'm thinking about future cases where we want to change error handling code, or any other auxiliary functionality we add to this package; it would be a shame for that to represent an API change given that virtually all consumers of this spec wouldn't be interested in those changes.

[kelunik]

I have changed the patch to allow only one single handler.

@WyriHaximus @joshdifabio @bwoebi Could you review please?

[WyriHaximus]

LGTM

[joshdifabio]

Yes, looks good.

[bwoebi]

Can we please return the old error handler here?

[kelunik]

Why do we need that? There should be only one handler per application.

[bwoebi]

To temporarily overwrite it, e.g. in a separate loop context.

Is there any harm in providing the possibility at all?

[kelunik]

If we do so, we should do the same for the loop error handler.

[bwoebi]

Good point, we should do that there too. [I honestly thought we already were doing that.]

[kelunik]

Done.