De-reference downstream promises when cancelled

[joshdifabio]

See the added test to understand what this optimisation is about. Essentially, I am proposing that when a downstream promise is cancelled, its handlers are removed from the upstream promise.

Prior to the changes to Promise, test which is included in this PR uses around 500MB of memory and takes around 30s to run on my machine using PHP5.5. With the patch, memory usage stays at around 13MB and the test takes approximately 4.5s to complete.

[joshdifabio]

Does anyone have any thoughts on this? Should I explain the issue and the changes further?

[clue]

Essentially, I am proposing that when a downstream promise is cancelled, its handlers are removed from the upstream promise.

I'm not sure I follow the implications of this. Doesn't this violate the core design of the cancellation support, i.e. handling what cancellation actually does is entirely up the the promise creator?

[jsor]

Sorry, for not commenting earlier. I've already played with your patch and i've come to the same conclusion as @clue.

This would change the cancellation behaviour in a non-backward-compatible way.

class CancelledException extends \Exception
{
}

$promise = new \React\Promise\Promise(
    function () {},
    function ($resolve, $reject) {
        $reject(new CancelledException());
    }
);

$childPromise = $promise->then();

$childPromise->otherwise(function (CancelledException $e) {
    echo "Cancelled\n";
});

$childPromise->cancel();
//$promise->cancel();

At the moment, this prints Cancelled for both $childPromise->cancel(); and $promise->cancel();, with your change it does not for $childPromise->cancel();.

I've played with some changes regarding cancellation semantics inspired by the changes bluebird did in it's 3.x release: http://bluebirdjs.com/docs/api/cancellation.html

So, that PR could be something we could incorporate for react/promise 3.0 if wenchange these semantics from "abortion" to "don't care".

[joshdifabio]

Thanks for taking a look at this, guys. I agree that this is a breaking change and wouldn't be suitable for 2.x.

Based on what is written in the docs for this package, I assumed that don't care was already the intended use for cancellation:

A cancellable promise provides a mechanism for consumers to notify the creator of the promise that they are not longer interested in the result of an operation.

It's not clear to me which of abortion or don't care is better or more useful in general, but I've found a couple of cases recently where long-lived promises leak lots of memory over time as they hold onto references to downstream resolvers for promises which have already been cancelled or resolved. In particular, the combinator functions such as race() cause leaks as resolvers which no longer serve any purpose continue to be referenced by upstream promises.

[jsor]

Yes, there's room for optimizations. I think we just leave this PR open until we have decided which route to go regarding cancellation behavior.

[jsor]

FYI: #56

[clue]

Thank you for digging into this and filing this PR! 👍

This is kind of an old issue that hasn't seen any activity in a while and the project has changed significantly in the meantime. It looks like this deals with the same memory issues that have been addressed in #113, #115, #116, #117, #118, #119, #123, #124 and others.

Given that, I'll assume this has been resolved in the meantime and will close this for now. If you feel this is still relevant, please come back with more details and we can always reopen this 👍

Again thank you for your effort nonetheless, keep it up! 👍