Skip to content

Follower cancellation propagation #99

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

Merged
merged 2 commits into from
Sep 19, 2017

Conversation

jsor
Copy link
Member

@jsor jsor commented Sep 5, 2017

This PR implements cancellation propagation for promises following another promise in the same way currently implemented for child promises created by then().

Consider the following code:

$root = new Promise(/* ...*/);

$child = $root->then();

$follower = new Promise(function($resolve) use ($root) {
    $resolve($root);
});

The behaviour is identical regarding resolution. Both $child and $follower follow $root which means, both will be fulfilled or rejected in the same way as $root.

The behaviour is different regarding cancellation though.

  • If $child->cancel() is called, $root is cancelled although $follower is still interested in the result.
  • If $follower->cancel() is called, cancellation is not propagated to $root.

This PR fixes that, so $follower propagates cancellation in the same way as $child.

@jsor jsor added this to the v2.6.0 milestone Sep 5, 2017
@WyriHaximus
Copy link
Member

Are you backporting this to 1.x as well?

@jsor
Copy link
Member Author

jsor commented Sep 6, 2017

@WyriHaximus I haven't planned that. I think you're asking for reactphp/http#213. If a version of react/promise is supported which doesn't have this patch, then cancellation propagation must be manually implemented, like here: https://github.com/reactphp/http/pull/213/files#diff-45da203c5746d523d3d75762c539b707R56

I took care to be backward compatible, so that a canceller registered with a follower takes precedence over the canceller from the parent promise: https://github.com/reactphp/promise/pull/99/files#diff-71f7b9a6df57aaf364b33eacad82e245R116

Copy link
Member

@WyriHaximus WyriHaximus left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

Copy link
Member

@clue clue left a comment

Choose a reason for hiding this comment

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

Good catch, change LGTM! 👍

Shouldn't this be filed against current master and then be backported for the v2 branch? Either way, I'm good with this :shipit:

@jsor
Copy link
Member Author

jsor commented Sep 19, 2017

@clue I'm not sure tbh what's common practice here. I've done that always by starting a patch for the latest released version and then porting down to older and/or up to unreleased versions.

@jsor jsor merged commit 179520e into reactphp:2.x Sep 19, 2017
@jsor jsor deleted the follower-cancellation-propagation branch September 19, 2017 13:02
clue added a commit to clue-labs/promise that referenced this pull request Jun 8, 2018
WyriHaximus added a commit that referenced this pull request Jun 8, 2018
@clue
Copy link
Member

clue commented Jun 11, 2018

For reference only: This PR has been reverted via #122 for this release and is planned for a future release instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants