amphp / amp

A non-blocking concurrency framework for PHP applications. 🐘
https://amphp.org/amp
MIT License
4.23k stars 255 forks source link

Sharing same exception catch block not possible between coroutines? #254

Closed ostrolucky closed 5 years ago

ostrolucky commented 5 years ago
<?php

use Amp\Loop;

require __DIR__ . "/../vendor/autoload.php";

$couroutine1 = \Amp\asyncCoroutine(function() {
    throw new \LogicException(2);
});

Loop::run(function () use ($couroutine1) {
    try {
        // code which might throw exception
        if (rand(0, 1)) {
            throw new \LogicException(1);
        }
        // however, this coroutine might also throw same kind of exception
        $couroutine1();
    } catch (LogicException $exception) {
        // I would like this catch block apply for couroutine too
        echo 'successfully catched exception';
    }
});

I would expect I end up in catch block for both cases. What should I do if I don't want to duplicate try {} catch {} blocks in both places?

kelunik commented 5 years ago

You shouldn't use asyncCoroutine if you plan to catch the exceptions, but coroutine instead. Then you need to yield the returned promise.

<?php

use Amp\Loop;

require __DIR__ . "/../vendor/autoload.php";

$couroutine1 = \Amp\coroutine(function() {
    throw new \LogicException(2);
});

Loop::run(function () use ($couroutine1) {
    try {
        // code which might throw exception
        if (rand(0, 1)) {
            throw new \LogicException(1);
        }
        // however, this coroutine might also throw same kind of exception
        yield $couroutine1(); // <-- note the yield here
    } catch (LogicException $exception) {
        // I would like this catch block apply for couroutine too
        echo 'successfully caught exception';
    }
});
ostrolucky commented 5 years ago

Ok I admit my example wasn't very clear. $couroutine is meant to be asynchronous and must be kept that way. Here's better example

<?php

use Amp\Loop;

require __DIR__ . "/../vendor/autoload.php";

$stdout = \Amp\ByteStream\getStdout();

$couroutine1 = \Amp\asyncCoroutine(function() {
    yield \Amp\ByteStream\getStdout()->write(1);
    yield new \Amp\Delayed(0);
    yield \Amp\ByteStream\getStdout()->write(2);
    throw new \LogicException(1);
});

$couroutine2 = \Amp\asyncCoroutine(function() {
    yield \Amp\ByteStream\getStdout()->write(3);
    yield new \Amp\Delayed(0);
    yield \Amp\ByteStream\getStdout()->write(4);
    throw new \LogicException(2);
});

Loop::run(function () use ($couroutine1, $couroutine2) {
    try {
        // code which might throw exception
        if (rand(0, 1)) {
            throw new \LogicException(3);
        }
        // however, these coroutines might also throw same kind of exception
        $couroutine1();
        $couroutine2();
    } catch (LogicException $exception) {
        // I would like this catch block apply for couroutines too
        echo 'successfully caught exception ' . $exception->getMessage();
    }
});

One of these outputs are expected:

jvasseur commented 5 years ago

The role of asyncCoroutine is explicitly to escape the scope so it's normal that you can't catch those excptions.

If what you are trying to do is run couroutine1 and couroutine2 in parallel what you should do is something like this :

<?php

use Amp\Loop;

require __DIR__ . "/../vendor/autoload.php";

$stdout = \Amp\ByteStream\getStdout();

$couroutine1 = \Amp\coroutine(function() {
    yield \Amp\ByteStream\getStdout()->write(1);
    yield new \Amp\Delayed(0);
    yield \Amp\ByteStream\getStdout()->write(2);
    throw new \LogicException(1);
});

$couroutine2 = \Amp\coroutine(function() {
    yield \Amp\ByteStream\getStdout()->write(3);
    yield new \Amp\Delayed(0);
    yield \Amp\ByteStream\getStdout()->write(4);
    throw new \LogicException(2);
});

Loop::run(function () use ($couroutine1, $couroutine2) {
    try {
        if (rand(0, 1)) {
            throw new \LogicException(3);
        }

        yield \Amp\Promise\all([
            $couroutine1(),
            $couroutine2(),
        ]);
    } catch (LogicException $exception) {
        echo 'successfully caught exception ' . $exception->getMessage();
    }
});
ostrolucky commented 5 years ago

yep that's closer :+1: