Skip to content

Conversation

@clue
Copy link
Member

@clue 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
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
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
@clue clue deleted the double-cancellation branch March 25, 2016 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants