Skip to content

Conversation

@WyriHaximus
Copy link
Member

@WyriHaximusWyriHaximus commented Jan 7, 2022

Since async() returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled sleep() call.

$promise = async(staticfunction (): int{echo'a'; await(sleep(2)); echo'b'; returntime()})(); $promise->cancel(); await($promise);

This builds on top of #15, #18, #19, #26, #28, #30, and #32.

@WyriHaximusWyriHaximus added the new feature New feature or request label Jan 7, 2022
@WyriHaximusWyriHaximus added this to the v4.0.0 milestone Jan 7, 2022
@azjezz
Copy link

I think this following case would fail 🤔

$promise = async(function (): int{echo'a'; await(async(function(): void{await(async(function(): void{await(sleep(2))})())})()); echo'b'; returntime()})();

@WyriHaximusWyriHaximusforce-pushed the cancel-fiber branch 3 times, most recently from 208a782 to ab2e7b8CompareJanuary 9, 2022 11:11
@clueclue mentioned this pull request Jan 25, 2022
@WyriHaximusWyriHaximusforce-pushed the cancel-fiber branch 5 times, most recently from 794c7d8 to 1dd9138CompareFebruary 20, 2022 22:33
@WyriHaximusWyriHaximus changed the title [EXPIRIMENTAL] Cancel fiberImprove async() by making its promises cancelableFeb 20, 2022
@WyriHaximusWyriHaximus requested review from clue and jsorFebruary 20, 2022 22:33
@WyriHaximusWyriHaximus marked this pull request as ready for review February 20, 2022 22:34
@WyriHaximusWyriHaximusforce-pushed the cancel-fiber branch 2 times, most recently from d86b691 to 8a65d99CompareFebruary 21, 2022 15:43
@SimonFrings
Copy link
Member

@WyriHaximus looks good to me, only thing I just noticed is that the description for cancellation is missing inside the functions.php.

@WyriHaximus
Copy link
MemberAuthor

@WyriHaximus looks good to me, only thing I just noticed is that the description for cancellation is missing inside the functions.php.

@SimonFrings done 👍

Copy link
Member

@clueclue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WyriHaximus Some great progress in here, love this feature and how beautifully it aligns with our existing cancellation logic (#9 / #13)!

Added a number of smaller remarks below, otherwise looks like an excellent addition! Semantics make a lot of sense to me, so I'd love to get this in as is, but I do see some room for follow-up discussions once we address reactphp/promise#56 in the future 👍

@WyriHaximus
Copy link
MemberAuthor

@clue Yes this one is the missing link we need to make this package fully fit into our ecosystem. As mentioned on some of your comments, I will create a follow up PR to utilize WeakMap and WeakReference where possible to put the garbage collector in charge of cleaning up promise <-> fiber references. But for the sake of landing this feature, and requiring more time to fully test the behavior there I didn't update this PR with that.

@WyriHaximusWyriHaximus requested a review from clueMarch 2, 2022 12:44
@clueclue changed the title Improve async() by making its promises cancelableImprove async() by making its promises cancellableMar 3, 2022
Copy link
Member

@clueclue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WyriHaximus Some good progress! I agree the WeakMap implementation can be left up to a future PR, only added a couple of remarks below, otherwise LGTM!

@WyriHaximusWyriHaximusforce-pushed the cancel-fiber branch 3 times, most recently from 9df8ce5 to f400d20CompareMarch 4, 2022 08:11
Since `async()` returns a promise and those are normally cancelable, implementing this puts them in line with the rest of our ecosystem. As such the following example will throw a timeout exception from the canceled `sleep()` call. ```php $promise = async(static function (): int{echo 'a'; await(sleep(2)); echo 'b'; return time()})(); $promise->cancel(); await($promise); ```` This builds on top of reactphp#15, reactphp#18, reactphp#19, reactphp#26, reactphp#28, reactphp#30, and reactphp#32.
clue
clue approved these changes Mar 4, 2022
Copy link
Member

@clueclue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WyriHaximus Thanks for the update, changes LGTM, so let's get this shipped! :shipit:

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new featureNew feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@WyriHaximus@azjezz@SimonFrings@jsor@clue@kelunik