Skip to content

Conversation

@clue
Copy link
Member

@clue clue commented Feb 17, 2016

  • Add cancellation support for resolve() and reject()
    • Remove internal timers
    • Reject resulting promise with RuntimeException
  • Add cancellation support for timeout()
  • Cancellation of input promise for timeout() is not affected

Closes #3, supersedes/closes #13

@clue clue added this to the v1.1.0 milestone Feb 17, 2016
@clue clue mentioned this pull request Feb 17, 2016
@jsor
Copy link
Member

jsor 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
Copy link
Member

jsor 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
Copy link
Member Author

clue 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
Copy link
Member Author

clue 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.

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
Copy link
Member Author

clue 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
Copy link
Member

LGTM :shipit:

@jsor
Copy link
Member

jsor commented Feb 29, 2016

:shipit:

clue added a commit that referenced this pull request Feb 29, 2016
@clue clue merged commit 65adff0 into reactphp:master Feb 29, 2016
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.

Support Promise cancellation

3 participants