-
-
Notifications
You must be signed in to change notification settings - Fork 17
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
Improve async()
by making its promises cancellable
#20
Conversation
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';
return time();
})(); |
9e5259c
to
77dac0c
Compare
77dac0c
to
83c90c2
Compare
208a782
to
ab2e7b8
Compare
794c7d8
to
1dd9138
Compare
async()
by making its promises cancelable
d86b691
to
8a65d99
Compare
@WyriHaximus looks good to me, only thing I just noticed is that the description for cancellation is missing inside the |
8a65d99
to
25810ab
Compare
@SimonFrings done 👍 |
25810ab
to
9dfeea4
Compare
There was a problem hiding this 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 👍
9dfeea4
to
24e5b93
Compare
@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. |
async()
by making its promises cancelableasync()
by making its promises cancellable
There was a problem hiding this 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!
9df8ce5
to
f400d20
Compare
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.
f400d20
to
262ef59
Compare
There was a problem hiding this 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!
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 canceledsleep()
call.This builds on top of #15, #18, #19, #26, #28, #30, and #32.