Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
clue opened this issue May 17, 2022 · 2 comments
Closed

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

clue opened this issue May 17, 2022 · 2 comments
Labels
BC break help wanted Extra attention is needed
Milestone

Comments

@clue
Copy link
Member

clue commented May 17, 2022

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 reactphp/promise#56 for discussion about cancellation semantics in Promise v3).

@clue
Copy link
Member Author

clue commented Jun 27, 2022

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
Copy link
Member Author

clue commented Jun 30, 2022

Closed via #54 and #55 :shipit:

@clue clue closed this as completed Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC break help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

1 participant