Requesting implementation of done()

[sm2017]

See https://github.com/reactphp/promise/issues/66 please I need guzzlehttp/promises implements done() https://github.com/reactphp/promise#done-vs-then

[schnittstabil]

@socketman2016 Do you have a use case for that? Doesn't ->wait() work for you?

As far as I can see, ->done($onFulfilled, $onRejected) can be simulated by:

/**
 * @throws Exception 
 */
function done($promise, $onFulfilled = null, $onRejected = null) {
    $promise->then($onFulfilled, $onRejected)->wait();
}

// instead of $promise->done():
done($promise);

Or, depending on your needs:

/**
 * @throws Exception 
 * @return Promise
 */
function throwRejected($promise) {
    return (new Promise())->resolve($promise->wait());
}

$promise = throwRejected($promise);

[joshdifabio]

@schnittstabil The functions you propose are both blocking.

[schnittstabil]

@socketman2016 wrote at https://github.com/reactphp/promise/issues/66#issuecomment-254120920:

My application handle errors with exceptions and I have a Uncaught exception handler to manage them, So sometimes I need to consume promise , something like done() to force promise throws exceptions instead of reject promise

@socketman2016 Details would be nice. Looks like a design issue IMO. If you already have error handlers, why don't you call them? I see no need to take a detour via php error mechanisms.

function existingErrorHandler($error) {…}

function promiseErrorHandler($error) {
    // do what ever you want, e.g.
   existingErrorHandler($error);
   // and/or
   trigger_error($error->message, …);
   // and/or
   die();
}

$promise->then(null, promiseErrorHandler);

[sm2017]

Do you have a use case for that? Doesn't ->wait() work for you?

Yes , I use event loop in my application , it is a PHP script that runs for months and it is not possible for me to use wait() and any other blocking function like sleep() because these blocking function impact my performance

So ->wait() and simulated functions don't works for me , In waiting time I can response to other requests

If you already have error handlers, why don't you call them?

For many reason I cannot do that , once is my script must not never exit , it runs for months (May be 5-6 months) . I cannot tell more details about my script because it is very complex and big .May be you ask why I do this , It is a mistake , but the reason is PERFORMANCE , my script can handle 10M request/sec in a cheap VPS. I know it is possible to trigger_error and dont exit but my exception handler works for each request independent , that means If request A and B is processing , when A throw exception , B doesn't affects so it is not possible for me to trigger_error

The best solution for my problem is done that implemented in reactphp/promise but I want to use third party library in my project (aws-sdk-php) that is base on guzzle/promises so sometimes I need uncaugh exception instead of rejected promise

[schnittstabil]

@socketman2016 I don't feel capable enough of implementing done in guzzle/promises, at least at the moment. However, possible implementors and the PO need to be convinced that done is needed and there's no better way to solve your problem. Moreover, it can take weeks or months until it gets merged and released. Thus please excuse me if I'm too bothersome.

I've used a similar approach the other day:

function error_handler($error) {
    // do logging stuff, etc.
}

function exception_error_handler($errno, $errstr, $errfile, $errline ) {
    errorHandler(new ErrorException($errstr, 0, $errno, $errfile, $errline));
}
set_error_handler("exception_error_handler");

// Therefore, it's easy to do:
$promise->then(null, error_handler);

I think your most important concern is performance, but calling done in your case looks like a detour to me:

1. you need to throw the exception
2. some php mechanism catch it
3. php determines your error handler, in my case exception_error_handler
4. php calls the error handler

Especially 2. and 3. seem inefficient and unnecessary to me.

I know it is possible to trigger_error and dont exit but my exception handler works for each request independent , that means If request A and B is processing , when A throw exception , B doesn't affects so it is not possible for me to trigger_error

To be honest, I don't understand why it is not possible. Anyway, I believe the approach above isn't affected by this.

The best solution for my problem is done…

This might be true, but done also has some drawbacks, it always returns null. Thus the following is not possible:

function onFulfilled() {
    if (…) throw …;
}

$promise->then(…)->done(onFulfilled)->then(function () {
  // ok, no error, do the next step
});

[sm2017]

Dear @schnittstabil, Really thanks for your answer and your offering solutions. About your offering solution, it should be noted that, it is a very nice and innovative solution, but sad to say it is not the solution of my problem because in my project the process of each request is done in an isolation manner and an error through a request may not affect the other requests; while your solution demolishes the property of being isolated for some reasons in my project.

I dont want to tell all reasons for verbosity , but one of the most important is :

Assume we want to run a HTTP server with PHP , when a new request is comming onMessage will be executed (For simplicity I just simulate my problem , it is not real problem)

public function onMessage($request){
      $this->process($request);
      $this->sendResponse();
}

public function process($request){
      //Part 1
      //Part 2
      //Part 3
      //Part 4
}

Assume Parts 1-4 is running in an asynchronous mode .Remember that each part can be either a promise or not . the problem is some of asynchronous codes are not promise .

We have 2 requests , A and B . Assume the following scenario

onMessage(A)
process(A)
Part 1(A)
onMessage(B)
process(B)
Part 2(A)
Part 2(B)
Part 3(B)
Part 3(A)
Part 4(A)
Part 4(B)

If there is no error in processing all things work good , But when we have an error in Part 2(A) what happens for B? To isolate I do as follows (In the real project it is very more complex than this)

public function onMessage($request){
   try{
       $this->process($request);
      $this->sendResponse();
   }catch(\Exception $e){
       //Send error and log
   }
}

So when Part 2(A) is thrown an exception , We send an error to A and stop running Part 3(A) and Part 4(A) but there is no effect on B

About the point that you have mentioned i. e. "done that always returns null", I have no concern because I do not need it.

I would be really thankful and glad if you consider the implementation of done in your planning and agenda. I impatiently wait for merging and releasing the new version. Sincerely yours

[schnittstabil]

the problem is some of asynchronous codes are not promise .

@socketman2016 Thanks for providing some insights. Maybe the following doesn't meet your requirements, I don't know how your non-promise asynchronous code looks like.

Personally, I would switch to a pure promise-based approach:

public function process($request){
    // we start with a promise, this can be done in many ways,
    // this one is rather opinionated, but sometimes quite helpful
    return (new Promise())->resolve(null)
    ->then(function ($result) {
        // Of course, $result is null
        // lets do a sync task
        //Part 1
        // if an exception is thrown within then-closures, the promise gets rejected
        $result = …;
        // to resolve the promise just return the result
        return $result;
    })->then(function ($result) {
        // $result is the return value of Part 1
        // (we don't get here, if Part 1 throws an error)
        //Part 2
        // lets assume this is a promise task
        $promise = doSomeAsyncWork($result);
        // we don't need to ->wait() or ->done(), just return the promise
        return $promise;
    })->then(function ($result) {
        // $result is NOT a promise, it's the resolved value of the Part 2 $promise
        // (we don't get here, if Part 2 was rejected)
        // lets do a non-promise async task, but first create a new promise:
        $promise = new Promise();
        //Part 3
        //…
                // somewhere, deep nested you get a result:
                $result = …;
                // and resolve your promise
                $promise->resolve($result);
        //…
        return $promise;
    });
}

public function onMessage($request){
    $this->process($request)
    ->then(function ($result) {
        // $result is the value of Part 3
        $this->sendResponse();                  
    }, function ($error) {
        // $error can be:
        // a thrown Exception, from e.g. Part 1
        // or the value of a rejected promise, e.g. Part 2 or Part 3
    });
}

[sm2017]

Pure promise-based approach has some drawbacks , in my application it is very hard coded.

As I told before , For simplicity I just simulate my problem , An in real project I have many other complicated challenges , So I dont want to migrate to pure promise-based approach .One of the most important challenge that prevent me to migrate to pure promise-based approach is joining asynchronous parts. I think in pure promise-based approach it will be spaghetti code

Consider the following scenario , To response request C , We must run part1 and part2 asynchronously then part2 will run part2_1 and part2_2 and part2_3 asynchronously then part2_2 will run part2_2_1 and part2_2_2 then all parts must join together to calculate right response(All of the part is running in a single thread)

Do you still think done() is not the best solution ?

edit: Also it may be dynamic that means in some situation there is no part2_2 or part_1 , Sometimes it is recursive that means part2_2_2 may be run part2 or not , So you can see I have many many complicated challenges , So I think pure promise-based approach is bad solution for me Notice : I am not allowed to use wait because it impact performance

[schnittstabil]

I think in pure promise-based approach it will be spaghetti code

Promises want to make working with asynchronous operations much more pleasant. Joining asynchronous parts is quite easy:

// forking
$promises = [];
$promises[] = createPromiseA();
$promises[] = createPromiseB();

// joining
Promise\all($promises)->then(function ($results) {
    list($valueA, $valueB) = $results;
    ...
});

It's a bit unfortunate that $result is an array, at first glance one may think the $promises are executed sequentially, which is of course not the case. See src/functions.php for even more powerful functions.

The above also gives a hint how modular code is written with Promises:

// instead of
$resultA = null;
createPromiseA()
    ->then(function ($valueA) use (&$resultA) {
        $resultA = $valueA;
        $resultB = 42;
        return $resultB;
    })
    ->then(function ($resultB) use (&$resultA) {
        // now we have access to $resultA and $resultB
    });

// we do
createPromiseA()
    ->then(function ($resultA) {
        $resultB = 42;
        return [
                'a' => $resultA,
                'b' => $resultB,
        ];
    })
    ->then(function ($results) use (&$resultA) {
        // now we have access to $results['a'] and $results['b']
    });

Do you still think done() is not the best solution ?

I don't want to proselytize you :wink: I think done() is maybe helpful in case of legacy code, and maybe it is useful enough to get implemented. However, I think it leads to code which is hard to reuse and hard to reason about, especially with error handling – I don't think Promises are the best way to write async code, but they are quite powerful.

Also it may be dynamic that means in some situation there is no part2_2 or part_1

Yeah, sometimes things get really complicated. On cheap trick to keep the order of results in the joined Promise:

// forking
$promises = [];
$promises[] = ($usePart1) ? createPromisePart1() : (new Promise())->resolve();
$promises[] = createPromisePart2();

// maybe this works too, I can't test it atm:
$promises[] = ($usePart1) ? createPromisePart1() : null;

// joining
Promise\all($promises)->then(…)

Sometimes it is recursive that means part2_2_2 may be run part2 or not

I would suggest a rather functional style, maybe you want to do it with more OO:

function part2_2_2($args) {
    return Promise\all([
        createPromise(),
        ($usePart2) ? part2($args) : (new Promise())->resolve()
    ]);
}

function part2($args) {
    return Promise\all([
        createPromise(),
        ($usePart2_2_2) ? part2_2_2($args) : (new Promise())->resolve()
    ])->then(function ($results) {
        // maybe you need to merge the results, e.g.
        return $results[0] + $results[1];
    });
}

// or if the condition is not known in advance
function part2($args) {
    return createSomePromise()
        ->then(function ($result) {
            if ($usePart2_2_2) {
                return part2_2_2(…);
            }
            // return null or whatever fits best
        });
}

[sm2017]

I think done is needed to be implemented

See https://github.com/reactphp/promise/issues/66 again , It it nice that guzzle/promises and reactphp/promise be compatible and can be converted together , As you know many third party library is base on reactphp/promise or guzzle/promises and now it is hard to work with both. My current problem is My project is based on done and now I cannot easily work with third party library that use `guzzle/promises``

I am agree with you that Promise\all() and src/functions.php are powerful , But I still think your approach to joining and recursive running is inefficient than my approach.

As you know Promise\all() just accept promise and only works in pure promise-based approach. If I accept that your approach is better , still I must works months to migrate to your approach.

My approach is based on callbacks and I have a Manager - Worker system , when you need async part you send it to Manager and when result is ready callbacks resume execution .Manager dont do any thing just manage Workers and Worker can be any thing , can be mathematical , file , database or any other operation By default , Only I have resolve callbacks and all errors is exceptions so It is very easier to handle errors , no need to code for each async part just throw exception like sync codes

All callbacks is a simple method in a class , this approach is async but you code like sync :grin:. To joining only Manager decide that "Is it the time that to call callback?" . To recursive running we do like sync codes

Consider following code

createPromiseA()
    ->then(function ($resultA) {
        $resultB = 42;
        return [
                'a' => $resultA,
                'b' => $resultB,
        ];
    })
    ->then(function ($results){
        // now we have access to $results['a'] and $results['b'] but here we dont need both
      $resultC = 43;
        return [
                'a' => $resultA,
                'b' => $resultB,
                'c' => $resultC,
        ];
    })->then(function ($results){
        // now we have access to $results['a'] and $results['b'] and $results['c'] but here we only need $results['b']
      $resultD = 43;
        return [
                'a' => $resultA,
                'b' => $resultB,
                'c' => $resultC,
                'd' => $resultD,
        ];
    })->then(function ($results){
        // now we have access to $results['a'] to $results['d'] here we needs all results
    });;

This is very hard coded always we must do a boring job , but in my approach , All thing is an isolated object and we can set properties like $this->resultA = 25

@mtdowling , @schnittstabil , @joshdifabio and any other possible implementors , Please implement done

[schnittstabil]

This is very hard coded always we must do a boring job ,

C'mon! Nobody would do it that way. The promise based approach forces one to follow some good practices, e.g. Exceptions are handle the same way as in sync code: they bubble up – at least if you don't use set_error_handler or similar.

class Results {
    // your domain specific class, e.g. Psr\Http\Message\ResponseInterface
}

// some tasks
function runTaskA($results) {
    $results->a = 41;
    return $results;
}

function runTaskB($results) {
    $results->b = 42;
    return $results;
}

function runTaskC($results) {
    $results->c = 43;
    return $results;
}

function runTaskD($results) {
    $results->d = 44;
    return $results;
}

// sync version
try {
    $result = new Results();
    $result = runTaskA($result);
    $result = runTaskB($result);
    $result = runTaskC($result);
    $result = runTaskC($result);
    // now we have access to $result->a to $result->d
} catch (Exception $err) {
    // error handling
}

// promise version
createPromise(new Results())
    ->then(runTaskA)
    ->then(runTaskB)
    ->then(runTaskC)
    ->then(runTaskD)
    ->then(function ($result){
        // now we have access to $result->a to $result->d
    })
    ->then(null, function ($err){
        // error handling
    });

You have been warned, I don't want to bother you anymore :wink: – back to your problem. You didn't mentioned what kind of event loop you use, but maybe you are using React\EventLoop. In that case you can throw the exception at the next tick:

First we need a dump helper class

use React\EventLoop\LoopInterface;

class ThrowAtNextTick
{
    protected $loop;

    public function __construct(LoopInterface $loop)
    {
        $this->loop = $loop;
    }

    protected function fixStacktrace($error)
    {
        if ($error instanceof Throwable || $error instanceof Exception) {
            return $error;
        }

        try {
            throw new Exception($error);
        } catch (Throwable $err) {
            return $err;
        } catch (Exception $err) {
            return $err;
        }
    }

    public function __invoke($reason)
    {
        $reason = $this->fixStacktrace($reason);
        $this->loop->nextTick(function () use ($reason) {
            throw $reason;
        });
    }
}

Then the nasty hacking

use GuzzleHttp\Client;
use GuzzleHttp\HandlerStack;
use WyriHaximus\React\GuzzlePsr7\HttpClientAdapter;

// bootstrapping
$loop = React\EventLoop\Factory::create();
$throwAtNextTick = new ThrowAtNextTick($loop);
$client = new Client([
    'handler' => HandlerStack::create(new HttpClientAdapter($loop)),
]);

// down the rabbit hole
$loop->nextTick(function () use ($client, $throwAtNextTick) {
    $promise = $client->getAsync('http://example.org')->then(function () {
        throw new Exception(42);
    });

    // instead of $promise->done():
    $promise->then(null, $throwAtNextTick);
});

// run the event loop
$loop->run();

[sm2017]

Too awesome answer !! ThrowAtNextTick and class Results are very nice. I will test the solution asap

But I cannot understand why in fixStacktrace we have

        try {
            throw new Exception($error);
        } catch (Throwable $err) {
            return $err;
        } catch (Exception $err) {
            return $err;
        }

Is the catch (Throwable $err) important? I think the following code is enough

        try {
            throw new Exception($error);
        } catch (Exception $err) {
            return $err;
        }

Or may be just

return new Exception($error);

Is right? or not?

I want to know what you think about trait , interface and Inheritance in my approach I can use all of them and for example override a method in child class to reuse my parent codes but in pure promise approach how is possible ?

// promise version
createPromise(new Results())
    ->then(runTaskA)
    ->then(runTaskB)
    ->then(runTaskC)
    ->then(runTaskD)
    ->then(function ($result){
        // now we have access to $result->a to $result->d
    })
    ->then(null, function ($err){
        // error handling
    });

createPromise(new Results())
    ->then(runTaskA)
    ->then(runTaskOveridedB)
    ->then(runTaskC)
    ->then(runTaskD)
    ->then(function ($result){
        // now we have access to $result->a to $result->d
    })
    ->then(null, function ($err){
        // error handling
    });

I think we have to repeat all steps A-D (Copy and Past) to runTaskOveridedB in pure promise approach but in my approach we only need yo override runTaskB

How is possible have a concept like trait or interface or abstract class , for example like in interface that we can force developer to implement methods , we force developer to runTaskA then runTaskB then runTaskC

[schnittstabil]

But I cannot understand why in fixStacktrace we have […] catch (Throwable $err) Or may be just

return new Exception($error);

In some edge cases new Exception($error) itself throws an error, thus catch (Throwable $err) seemed reasonable to me:

new Exception(new stdClass);
// PHP 5.6 triggers a fatal Error:
// PHP Fatal error:  Wrong parameters for Exception([string $exception …])

// PHP7 throws a Throwable:
// PHP Fatal error:  Uncaught Error: Wrong parameters for Exception([string $message …])

Btw, if you are using GuzzleHttp\Promise\Promise directly, don't forget to run the task queue on each tick of the loop, otherwise your Exceptions get lost:

use function GuzzleHttp\Promise\queue;

$loop->addPeriodicTimer(0, [queue(), 'run']);

[schnittstabil]

I want to know what you think about trait , interface and Inheritance in my approach I can use all of them and for example override a method in child class to reuse my parent codes.

Using inheritance just for the sake of code reuse is almost always bad practice, it usually violates the Liskov substitution principle (LSP). That was one of the main reasons why traits were invented. That said, feel free to mix functional style (Promises, callbacks, …) and object orientated style (interfaces, traits, …) as you like.

However, what I can't recommend is mixing it with pure imperative style as well (global variables, global error handlers etc.): Let's assume a colleague wants to reuse your Manager and some of your Workers. He have to setup these global variables – ugh. But even worse, he have to setup the global error handler as well. And if he already uses set_error_handler for his own business logic, things get really complicated.

One possible option comes from Node.js: They extensively use callbacks for async purposes, and consider errors as results, thus they introduced callback conventions:

The asynchronous form always takes a completion callback as its last argument. The arguments passed to the completion callback depend on the method, but the first argument is always reserved for an exception. If the operation was completed successfully, then the first argument will be null or undefined:

fs.readFile('/etc/passwd', function (err, data) {
  if (err) {
    console.error(err);
    return;
  }
  console.log(data);
});

I think we have to repeat all steps A-D (Copy and Past) to runTaskOveridedB in pure promise approach but in my approach we only need yo override runTaskB

Personally, I wouldn't extend the Promise class, if that is what you have in mind. I would use Promises only as return value, e.g.:

class Worker {
    public function sum($val1, $val2) {
        $promise = new Promise();
        $promise->resolve($val1 + $val2);

        return $promise;
    }

    public function mult($val1, $val2) {
        $promise = new Promise();
        $promise->resolve($val1 * $val2);

        return $promise;
    }

    public function __invoke($val1, $val2) {
        return $this->mult($val1, $val2)->then(function ($result) {
            return $this->sum($result, 42);
        });
    }
}

$worker = new Worker();
$worker(1, 2)->then(…);

Furthermore, don't worry about mixing callback and promise style. The nodejs convention allows us to convert Workers back and forth:

$callbackWorker = function ($val1, $val2, callable $cb) {
    // return some value
    $cb(null, $val1 + $val2);
    // or return null
    $cb();
    // or *throw* an error
    $cb(new Exception());
};

function promisify(callable $cbWorker) {
    return function () use ($cbWorker) {
        $promise = new Promise();
        $args = func_get_args();

        // add callback:
        $args[] = function ($err = null, $value = null) use ($promise) {
            if ($err) {
                $promise->reject($err);
            } else {
                $promise->resolve($value);
            }
        };

        call_user_func_array($cbWorker, $args);

        return $promise;
    };
}

$promiseWorker = promisify($callbackWorker);

$promiseWorker(1,2)->then(function ($value) {
    echo $value;
});

Or the other way around:

$promiseWorker = function ($val1, $val2) {
    $promise = new Promise();
    $promise->resolve($val1 + $val2);

    return $promise;
};

function callbackify($pWorker) {
    return function () use ($pWorker) {
        $args = func_get_args();
        $cb = array_pop($args);
        $promise = call_user_func_array($pWorker, $args);

        $promise->then(function ($value) use ($cb) {
            $cb(null, $value);
        }, function ($reason) use ($cb) {
            $cb($reason);
        });
    };
}

$callbackWorker = callbackify($promiseWorker);

$callbackWorker(42, 43, function ($err = null, $value = null) {
    if ($err) {
        echo 'ERROR: '.$err;
    } else {
        echo $value;
    }
});

[sm2017]

Please thumbs up this post if you think "implementation of done()" is needed 👍

[stefanotorresi]

I don't think done() needs to be implemented, but I think there should be a mechanism of some sort to let exceptions bubble out of promise chains. In ReactPHP this can be achieved by calling done() at the end of a chain. Is there a canonical way to do so with guzzle promises?

[shtse8]

I have the same situation. And two different promises (React & Guzzle) are using at the same time makes things messed up. Any suggestions?