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

Support promise cancellation #18

Merged
merged 4 commits into from Feb 29, 2016

Conversation

@clue
Copy link
Member

commented Feb 17, 2016

  • Add cancellation support for resolve() and reject()
    • Remove internal timers
    • Reject resulting promise with RuntimeException
  • Add cancellation support for timeout()
    • Only cancel input promise (see #13 for some background, thanks @jsor)
  • Cancellation of input promise for timeout() is not affected

Closes #3, supersedes/closes #13

@clue clue added the new feature label Feb 17, 2016

@clue clue added this to the v1.1.0 milestone Feb 17, 2016

@clue clue referenced this pull request Feb 17, 2016
@jsor

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

👍


For the record (not suitable right now because it'd be a BC), i recommend to not typehint against PromiseInterface but use Promise\resolve to turn any input into a Promiseinstance.

namespace React\Promise\Timer;

use React\EventLoop\LoopInterface;
use React\Promise;

function timeout($input, $time, LoopInterface $loop)
{
    $promise = Promise\resolve($input);

    return new Promise\Promise(function ($resolve, $reject) use ($loop, $time, $promise) {
        $timer = $loop->addTimer($time, function () use ($time, $promise, $reject) {
            $reject(new TimeoutException($time, 'Timed out after ' . $time . ' seconds'));
            $promise->cancel();
        });

        $promise->then(function ($v) use ($timer, $loop, $resolve) {
            $loop->cancelTimer($timer);
            $resolve($v);
        }, function ($v) use ($timer, $loop, $reject) {
            $loop->cancelTimer($timer);
            $reject($v);
        });
    }, array($promise, 'cancel'));
}

The advantages are, that you can remove a few if's and more important, it will be more interoperable once Promise\resolve supports turning foreign promises into React promises.

@jsor

This comment has been minimized.

Copy link
Member

commented Feb 24, 2016

My, bad. I see now that you're doing the $promise instanceof CancellablePromiseInterface checks because React\Promise ~1.1 is supported.

@clue

This comment has been minimized.

Copy link
Member Author

commented Feb 25, 2016

i recommend to not typehint against PromiseInterface but use Promise\resolve to turn any input into a Promiseinstance

Thanks for the suggestion @jsor. I think it makes sense to consider this, despite this being unrelated to this ticket, so I've just filed #19 to keep track of this.

@clue

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2016

I'd like to get in reactphp/promise#48 first so that we can rely on cancellation support to be present in all installations and hence make this feature more predictable. An update to this PR is ready, I'll push this once the other PR is in and a v1.2.0 has been tagged.

More predictable cancellation by requiring cancellation support
Cancellation support has been backported from promise v2.1 to v1.2,
so we can now rely on cancellation support being available for both
major versions.
@clue

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2016

I'd like to get in reactphp/promise#48 first so that we can rely on cancellation support to be present

The linked PR is now in and I've updated this PR to make this feature more consistently available.

Ready for review :shipit:

@WyriHaximus

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

LGTM :shipit:

@jsor

This comment has been minimized.

Copy link
Member

commented Feb 29, 2016

:shipit:

clue added a commit that referenced this pull request Feb 29, 2016
Merge pull request #18 from clue-labs/cancellation2
Support promise cancellation

@clue clue merged commit 65adff0 into reactphp:master Feb 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.