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

Cancellable promise #20

Merged
merged 8 commits into from
Oct 15, 2014
Merged

Cancellable promise #20

merged 8 commits into from
Oct 15, 2014

Conversation

jsor
Copy link
Member

@jsor jsor commented Oct 4, 2014

Added CancellablePromiseInterface implemented by all promises.

Note on the implementation:

The $canceller callback is responsible for rejecting the promise, there is no automatic rejection mechanism (See https://github.com/reactphp/promise/tree/cancellable-promise#promise-1).

The $canceller callback is invoked only, if cancel() is called either on the root promise or on all derived promises.

Called on root promise

$promise = getCancellablePromise();

$promise->cancel(); // Promise is cancelled.

Called on derived promises

$promise = getCancellablePromise();

$derived1 = $promise->then();
$derived2 = $promise->then();

$derived1->cancel(); // $promises is not cancelled
$derived2->cancel(); // $promises is cancelled

@jsor jsor added this to the v2.1 milestone Oct 4, 2014
@mtdowling
Copy link
Contributor

Because a Deferred value is the thing that is waiting to be resolved or rejected, I would imagine that a deferred value would also be the thing that is cancelled, not a promise (from what I can tell, that's what when.js is doing too, though it's marked as deprecated).

What is the reasoning for adding this to the library vs just having the users of this library implement a method for cancelling deferred values?

@jsor
Copy link
Member Author

jsor commented Oct 5, 2014

I'm not sure i understand the question, can you elaborate?

A Deferred represents a computation. The promise represents the result of that computation, it is the value of a Deferred.

Calling cancel() on a Promise signals the producer of the promise (which controls the Deferred) that there is no longer interest in this value.

The producer may implement a mechanism to cancel the computation.

function readLargeRemoteFile()
{
    $canceller = function($resolve, $reject) use (&$request) {
        $request->abort();
        $reject(new RequestAbortedException());
    };

    $deferred = new Deferred($canceller);

    $request = (new Client())->request();

    $request->then([$deferred, 'resolve'], [$deferred, 'reject']);

    return $deferred->promise();
}

$promise = readLargeRemoteFile();
$promise->cancel();

return new static($this->resolver($onFulfilled, $onRejected, $onProgress));
$this->requiredCancelRequests++;

return new static($this->resolver($onFulfilled, $onRejected, $onProgress), function ($resolve, $reject, $progress) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you consider adding a check to conditionally return this new behavior vs the old behavior based on if a canceller is associated with the promise? Creating a new closure when it isn't necessary would add overhead where it isn't going to be utilized.

Something like:

if (!$this->canceller) {
    return new static($this->resolver($onFulfilled, $onRejected, $onProgress));
}

// Return the new stuff you're adding here...

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, done in 77f78d9

@mtdowling
Copy link
Contributor

My point was that up until now, a promise was delivered a value from a deferred, and there was no way to send a message from the promise to the deferred (e.g., cancelling a computation). Are other promise implementations doing anything similar?

@jsor
Copy link
Member Author

jsor commented Oct 5, 2014

Yes, that was also my concern. Many implementations immediately cancel the promise which gives promise consumers resolver capabilities.

Because of that, i decided to handle cancellelations like notifications for the producer which can then decide to cancel, reject or just do nothing.

Popular promise libraries which implement cancellable promises are Bluebird, WinJS, Dojo

@jsor
Copy link
Member Author

jsor commented Oct 8, 2014

+1 from @cboden, @clue and @WyriHaximus via IRC.

@cboden Ready to merge.

@cboden
Copy link
Member

cboden commented Oct 8, 2014

Will review this weekend.

@cboden cboden self-assigned this Oct 8, 2014
@jsor
Copy link
Member Author

jsor commented Oct 8, 2014

Oh, yes. Sorry for the confusion 😊

cboden added a commit that referenced this pull request Oct 15, 2014
@cboden cboden merged commit 41fdcef into master Oct 15, 2014
@@ -1,6 +1,11 @@
CHANGELOG
=========

* 2.1.x (xxxx-xx-xx)
Copy link
Member

Choose a reason for hiding this comment

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

Just remember to update this before tagging. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

yep 😎

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