Single onError handler

[kelunik]

I think there should only be one possible onError handler. It should be set be the application only, never by libraries. Currently we return an event identifier as well and can cancel it.

@trowski and @rdlowrey already agreed in chat, but just confirm that with a :+1:

[AndrewCarterUK]

If we're doing this I think that we should change the name so that it clearly operates in a different mechanism to the other on* methods.

[kelunik]

It's still somehow an event. But I'd also be fine with setErrorHandler.

[bwoebi]

I'd like to note that I'm neither really in favor nor against it. I've not seen a need for multiple error handlers yet, but I might be wrong.

The only issue is that while we always can allow for multiple invocations of onError, we cannot restrict it later without major version bump.

onError in the LoopDriver though always should only accept one callable. (i.e. if we allow multiple onError callbacks, we'd just pass a closure from the Loop class which iterates over the list of callbacks).

Thus I'm slightly leaning towards the restricted version so that we can still allow more in case we need to. We should restrict it at the Loop class level too.

[kelunik]

I think it's a bad idea to have multiple handlers, because it's a last chance handler. Similar to the one in PHP. If that handler throws again, the event loop breaks and stops.

[bwoebi]

I principally agree with you; I'm just not sure whether there's really no need for it.

Hence it should be designed in a way that allows a change in a minor version (i.e. without BC break).

[kelunik]

That's not possible. Because multiple invocations will replace the current handler in the one thing and set another handler in the other thing.

[bwoebi]

Uhm… you want to allow to overwrite the handler (via Loop class)? I think that's a bad idea and encourages mistakes (accidental overwrite, people misunderstanding that it cannot be chained…)

[kelunik]

You might want to unset it.

[bwoebi]

Don't think so. If you need to unset, also unset the whole loop then.

[kelunik]

Maybe you want to set it in Aerys for startup and then let the user override it if he wants to log the errors himself.

[bwoebi]

You'd then provide your own way to the user to access the errors. (in the callback, call another user-provided callback)

[WyriHaximus]

setErrorHandler is more explicit then onError for an error handler that only allows one listener, :+1: on this whole idea

[rdlowrey]

There exists an unambigous a need for an event-loop-global error handler. We've felt this need most acutely in the aerys web server where an uncaught exception inside the event loop can bring down a server process. Indeed, that was the reason we first added the onError method to the amp reactor implementation. For the same reason set_exception_handler() is useful in web SAPI applications we need to be able to deal with scenarios that would otherwise end our application. In an async event-driven application the event loop is essentially its own VM environment inside the php process and it's critical to be able to deal with errors at the VM layer on a per-application basis.

As for nomenclature, I think it depends on whether we or not want to allow multiple handlers for such an event. If we want to allow users to register an arbitrary number of handlers then onError() likely makes sense. If one only then setErrorHandler() seems more appropriate.

[AndrewCarterUK]

Submitted a PR (#45) as we seem to have achieved close to a consensus:

There was some talk in the discussion about whether you would be able to enable/disable/reference/unreference this and treat it like other events.

This PR doesn't do that, because I think it raises a few complications (are error handlers automatically unreferenced?) that might lead to bugs and ultimately doesn't add any functionality.

If anyone disagrees with my assessment, feel free to thumbs down here and we'll move the discussion back to the issue.

[AndrewCarterUK]

Closed with #45