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 handler must only be called once, ignore double cancellation #47

Merged
merged 2 commits into from Feb 26, 2016

Conversation

@clue
Copy link
Member

clue commented Feb 25, 2016

As a first step, this ticket aims to document the current behavior. Following, we should fix the first test case and probably also reconsider the second one.

Opening this ticket as an RFC and for the reference.

@jsor

This comment has been minimized.

Copy link
Member

jsor commented Feb 26, 2016

Thanks for spotting. This should be easy to fix.

diff --git a/src/Promise.php b/src/Promise.php
index 75379ea..7af9943 100644
--- a/src/Promise.php
+++ b/src/Promise.php
@@ -91,7 +91,10 @@ class Promise implements ExtendedPromiseInterface, CancellablePromiseInterface
             return;
         }

-        $this->call($this->canceller);
+        $canceller = $this->canceller;
+        $this->canceller = null;
+
+        $this->call($canceller);
     }

     private function resolver(callable $onFulfilled = null, callable $onRejected = null, callable $onProgress = null)
@clue clue changed the title [RFC] Unexpected behavior for double cancellation Cancellation handler must only be called once, ignore double cancellation Feb 26, 2016
@clue

This comment has been minimized.

Copy link
Member Author

clue commented Feb 26, 2016

Thanks for spotting. This should be easy to fix.

Yeah, I already prepared the same fix for this issue, but figured I'd bring this up here first and confirm if this is actually the desired behavior. Documentation on cancellation behavior is scarce, also with respect to other promise implementors.

FWIW, I've encountered this issue while implementing #48, to which I've just pushed the same update.

This is now ready for review :shipit:

jsor added a commit that referenced this pull request Feb 26, 2016
Cancellation handler must only be called once, ignore double cancellation
@jsor jsor merged commit 74d2bb4 into reactphp:master Feb 26, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@clue clue deleted the clue-labs:double-cancellation branch Mar 25, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.