Review

[kelunik]

Could you please all review the full specification including all documents and documentation comments and post a comment afterwards here? Please review carefully and don't just put your thumb up here.

@trowski @assertchris @AndrewCarterUK @bwoebi @WyriHaximus @rdlowrey @sagebind @jsor

[codedokode]

May I add a note too? I am a user of promises, not developer though.

I don't like global error handler and using PHP error handling system.

Global error handler can cause issues if several third party libraries will set their own error handlers and start captching each other's errors.

I also don't like using PHP error (E_USER_ERROR) instead of exception. PHP errors are not fatal by default so it means that even if program has an error it will countinue working (I think that PHP errors/warnings/notice system is inconvenient and should be replaced with exceptions).

Also I don't like the idea that when() has a single callback for both successful result and an exception. This means that a developer would have to write if in every callback. Having separate callbacks might be more convenient (and if an error callback is not given an exception could be thrown).

If the user didn't handle the error it is currently silently ignored. I think it would be better if the errors were not ignored by default and user has to specify explicitly that they should be ignored. As it is with synchromous calls and exceptions - one has to put try to ignore the exception.

[kelunik]

Global error handler can cause issues if several third party libraries will set their own error handlers and start captching each other's errors.

No library should ever set that error handler. It should only be done by applications.

I also don't like using PHP error (E_USER_ERROR) instead of exception. PHP errors are not fatal by default so it means that even if program has an error it will countinue working (I think that PHP errors/warnings/notice system is inconvenient and should be replaced with exceptions).

Exceptions can't be reasonably be used here, as they'd bubble up into the wrong place. They'd bubble up into the resolution code and and error to consume a promise should never affect its resolution.

The error level is something that could be argued about indeed. Could be a E_USER_WARNING and only E_USER_ERROR in case the error handler throws.

If the user didn't handle the error it is currently silently ignored. I think it would be better if the errors were not ignored by default and user has to specify explicitly that they should be ignored. As it is with synchromous calls and exceptions - one has to put try to ignore the exception.

How do you want to achieve that?

In general @trowski, @bwoebi and me think that coroutines in form of generators are the preferred form of consuming promises. With coroutines try / catch works as in synchronous code and it's not possible to ignore any errors.

Additionally, when is error-first specifically to make it hard to ignore errors. If you want to consume the value, you also have to define the error parameter. So if you ignore the error, you won't do that by accident.

[codedokode]

In many cases there is no need to handle error - it should just terminate the application. For example, if a wget- like client cannot resolve DNS name or connect to a remote host then nothing can be done - the program should just terminate.

When writing synchronous code, exceptions provide this kind of error handling by default so the developer doesn't have to write any code for the case of an error:

$ipAddr = resolveDnsName($hostname); // throws on error
$socket = connectToHost($ipAddr, $port); // throws on error

This allows even nesting function calls:

$socket = connectToHost(resolveDnsName($hostname), $port); 

And this works even if the developer didn't think about handling errors at all (or didn't have time for that). Without exceptions, one would have to add if statement after every function call that can fail.

With current specification there is no error handling by default. The developer would have to write ifs for every asynchronous operation. So, would not it be better if default would be throwing an exception (even if it is thrown not where it is created) and some method to opt-in for handling this exception? So we could write something like this:

resolveDnsNameAsync($hostname)->
when(function ($ipAddr) use ($port) {
      return connectToHostAsync($ipAddr, $port);
})->when(function ($socker) {
   // do something with the socket
});

And have any errors handled by default. Otherwise the developer would have to write extra code.

Rearding the problem with throwing an exception not where it was generated, I don't know how to solve that. In PHP7 is possible to create exceptions not inherited from \Exception and they can save stack trace when created, not when thrown.

In general @trowski, @bwoebi and me think that coroutines in form of generators are the preferred form of consuming promises.

Can you please give a link to some article with examples of code?

I also would like to add that Hack (a version of PHP by Facebook) has special await syntax for handling asynchronous operations: https://docs.hhvm.com/hack/async/introduction .

[kelunik]

Can you please give a link to some article with examples of code?

https://github.com/amphp/amp + http://blog.kelunik.com/2015/09/20/getting-started-with-amp.html

The blog post covers Amp v1.0, but Amp v2.0 is about to be released. In Amp v2.0 you'd write async code like that:

<?php

Amp\Loop::run(function () {
    $ip = yield resolveDns("github.com");
    $socket = yield Amp\Socket\connect("tcp://{$ip}:80");
});

In case anything fails, an exception will be thrown just like in synchronous code and the program will be aborted. This is of course just an example, Amp\Socket\connect would usually do the DNS lookup for you.

[assertchris]

@kelunik other than #47, looks good to me.

[codedokode]

1) Regarding error handlers: PHP already has 2 global error handlers. Those are error handler set via set_error_handler() and uncaught exceptions handler set via set_exception_handler(). I don't like the idea to add third global error handler for specific type of errors (exceptions thrown inside a handler).

As I understand this project is supposed to be something that would unite different async libraries. New global error handler looks more like a separate library that can be useful to some projects but not to all of them.

Generally I don't like adding global state with this error handler.

2) Also as I understand the default action if the async operation has failed is to ingore the error. It might be more convenient if default action would be to throw an exception that would terminate the program. In many cases there is nothing that can be done to recover from the error so terminating the program is needed.

The same applies to exceptions thrown in an error handler. If something went wrong it might the best idea to terminate the program by default (if the developer is not interested in handling errors) rather than continue executing it to produce wrong results.

This is already used in synchronous operations. For example if PDO instance cannot establish connection to a database, it throws an exception that terminates the program (and writes a message to the error log or stderr). Doesn't it make sense to have the same default behaviour for asynchronous operations?

Additionally, when is error-first specifically to make it hard to ignore errors. If you want to consume the value, you also have to define the error parameter.

But this means having to add the boilerplate code to every handler.

3) Also I think that successful result processing and error handling could use different code paths. With this proposal they are both passed via the same function. But why not make 2 separate handlers? It could look like this:

interface AsyncResult
{
    // If the operaion has finished successfully
    public function onSuccess(callable $fn);

    // If the operation has failed
    public function onError(callable $fn)
}

Error handler could be set using onError() method, and if the developer doesn't want to handle errors himself and no error handler is set then an exception is thrown that will probably terminate the program.

So those developers who don't need custom error handling don't have to write any code for handling errors.

Here onError() handler is responsible only for reporting a failed async operation, not for handling exceptions thrown in onSuccess() handler.

This design has a problem though: onError() handler has to be set before starting asynchronous operation otherwise there will be a race condition. And one has to set both handlers to know when the operation has finished.

4) I don't know what is the best way to handle exceptions thrown inside a handler. It is a complicated question.

As I understand catching exceptions is necessary to make all handlers independent from each other so failure in one handler doesn't prevent other from running. But there still are cases when this won't work. For example, a handler could call die() or mess with global state.

React Promise library catches them and rejects the promise returned from then() method. I think it is a bad idea because those exceptions often get lost and not even reported to error logs. It makes debugging the program difficult if it uses a lot of promises (I had the experience of such debugging some time ago).

5) With this proposal throwing an exception in a handler doesn't terminate the program which is different from how exceptions work in synchronous code. For example this code looks like it terminates the program in case of error but in fact it doesn't:

$result->when(function ($e, $value) {
    if ($e) {
        throw new OperationFailedError($e);
    }

    ...
});

I think it is confusing and unexpected. Especially if exception indicates a developer's error (for example calling a function with invalid argument from a handler). Then this function terminates the program when called from synchronous code and doesn't if called from async code.

[codedokode]

Also I always use an error handler that converts PHP errors, warnings and notices to exceptions (similar to the one described in PHP manual on ErrorException). So in my code any PHP warning turns into a fatal error that terminates the program. But with this proposal those errors are not fatal if they happen in asynchronous code.