reactphp / async

Async utilities and fibers for ReactPHP.
https://reactphp.org/async/
MIT License
204 stars 19 forks source link

Cancellation semantics for `async()` and `coroutine()` #42

Closed clue closed 2 years ago

clue commented 2 years ago

What should the following code return?

$promise = async(static function (): int {
    try {
        await(React\Promise\Timer\sleep(2));
    } catch (Exception $e) {
        return 1;
    }

    return 2;
})();

$promise->cancel();

var_dump(await($promise));

At the moment, the code would print 1. The same cancellation logic is triggered for fibers (#20) and coroutines (#13). Accordingly, this affects Async v3 and Async v4.

Perhaps we should reject the entire promise? (See also https://github.com/reactphp/promise/issues/56 for discussion about cancellation semantics in Promise v3).

clue commented 2 years ago

Cancellation semantics are tricky. @WyriHaximus has set up this poll on Twitter: https://twitter.com/WyriHaximus/status/1539183900668440576

In accordance with this, it's currently my understanding each of these examples should behave exactly the same and only cancel the first sleep(2) and in effect keep executing sleep(3):

$promise = async(static function (): int {
    try {
        await(React\Promise\Timer\sleep(2));
    } catch (Exception $e) {
        await(React\Promise\Timer\sleep(3));
    }
})();

$promise->cancel();
$promise = coroutine(static function (): Generator {
    try {
        yield React\Promise\Timer\sleep(2);
    } catch (Exception $e) {
        yield(React\Promise\Timer\sleep(3);
    }
});

$promise->cancel();
$promise = React\Promise\Timer\sleep(2)->catch(function (Exception $e) {
    return await(React\Promise\Timer\sleep(3));
});

$promise->cancel();

Arguably, this not how we would recommend handling promise cancellations and it's also not something we're aware of in the wild at the moment. Consumers are advised to handle only appropriate rejections and reconsider if more operations are to be performed if either operation is cancelled.

I'll file PRs to adjust coroutine() and async() behavior to match the above logic by cancelling only the first operation for the outstanding v3 and v4 releases and link this against this ticket. This way, everything is in line with how promise cancellation has always worked ever since its inception in 2014.

clue commented 2 years ago

Closed via #54 and #55 :shipit: