Memory leaks with combinator functions

[joshdifabio]

I have uploaded a gist which shows a problem I've encountered recently when using promises and combinators: specifically, using a combinator function (particularly first()/race()/any()) will cause redundant resolver callbacks to remain attached to whichever promises do not 'win' the race to resolve the combined promise. The current Awaitable interface does not provide any mechanism to overcome this issue (e.g. some kind of cancel() or dispose() method). What do others think about this issue?

I don't personally think combinators should implicitly abort 'losing' promises (which is what cancel() sometimes means) but I do think it might be nice to have some kind of dispose() method:

interface Awaitable
{
    public function when(callback $onResolved): Disposable;
}

interface Disposable
{
    /** Inform the awaitable that you no longer care about its result and it can remove your callback **/
    public function dispose();
}

I'm guessing people will be against such a change, but it's come up for me a couple of times so I'm interested to hear people's thoughts.

[kelunik]

Promises can't be aborted. And we always have to attach a handler.

To support something like Disposable, we'd have to issue identifiers for all when calls, as each handler should be independent.

[kelunik]

Where's the real memory leak now? Promises usually don't exist for a long time after they have been resolved, as they're useless then.

[kelunik]

What we could do is something like when returning an identifier and providing a cancel method in Awaitable that cancels a when, but not the awaitable itself.

[joshdifabio]

Yeah, I guess that'd be more efficient than creating an object for every when() call, and more consistent with Loop as well.

[kelunik]

We don't need an object, we could just increment a counter that's used as key for private $whens.

[joshdifabio]

I agree, that would be a better approach.

[bwoebi]

I sometimes, but rarely, have that need. In these cases, it typically is just fine to handle the abortion in the callback directly, i.e.

function ($ex, $val) {
    if ($this->aborted) {
       return;
    }
    /* work with $ex and $val */
}

This usually has the advantage of not explicitly cancelling something on cancellation site, but being explicit of ignoring under certain circumstances [and you possibly still might want to log possible errors nevertheless] (it's hard to think about all the things to be cleaned up and easy to forget one).

Furthermore, you're saying it leaves the callbacks attached … but is that a problem? It isn't a memory leak as the other Promises will be resolved at some point (if they aren't that's a bug in your program). Nor is it a performance hit as calling a method to remove the callback is about just as expensive as calling a callback which immediately returns.

In your specific example the when() callback just temporarily "leaks" but the Awaitable impl should free it upon resolution. Typically not much memory is used here … in an extreme case maybe 500 bytes for a fullblown Closure [including statics from use ()] (and IMO if you have millions of Promises pending concurrently, you're not on a 4 GB RAM machine, but on something much beefier...). There's usually much more memory used for the actual action behind. For example two dns lookups on different dns servers racing against each other … each lookup is having it's own structure, watcher callback, whatever… that's typically much more expensive and still counting twoards your "leaked" memory.
In case this gets too expensive, you should consider adding a real cancel() method [not to the standard, but to the class you started the operation on] which then aborts the lookup handling and resolves the Awaitable immediately.

Thus, unless there are stronger arguments, I'm pretty much opposed to adding this. The way it is currently is much cleaner IMO.

[joshdifabio]

Sorry guys, I was replying via email on my phone last night and I missed a bunch of what you said.

I'm not sure if I've been clear with what I'm saying, so allow me to try and clarify with a more concise, albeit artificial, example than the one I posted in a gist yesterday:

while (true) {
    yield race(doSomething(), $someVeryLongLivedPromise);
}

In this example, $someVeryLongLivedPromise is a promise which takes a long time to be resolved; it could take hours, days or weeks to resolve. Each time race() is called, it adds a callback to each of the promises it's been provided. Imagine that doSomething() is resolved within a second or so; the callback attached to $someVeryLongLivedPromise by race() is now redundant, as the 'race' was already won by doSomething(). Over time, $someVeryLongLivedPromise will accumulate millions of redundant callbacks in this way.

To be crystal clear, if doSomething() generally resolves very quickly and $someVeryLongLivedPromise takes a long time to resolve then this example can waste a lot of memory very quickly. It will also make eventual resolution of $someVeryLongLivedPromise extremely slow as it will have to call millions of callbacks.

The suggestion is that some method could be provided for cases like this so that race() could remove the callback it attached to $someVeryLongLivedPromise once the race has been won by doSomething().

As suggested by @kelunik, something like the following would be a more efficient way of implementing this than my earlier suggestion, although I'm not sure about naming the new method cancel() as this would conflict with existing cancel() methods on React, Guzzle and Icicle promises which have a different meaning to what I'm proposing:

interface Awaitable
{
    /**
     * @return string A unique (to this awaitable) watcher ID which can be passed to cancel()
     **/
    public function when(callable $callback) : string;

    public function cancel(string $watcherId) : void;
}

However, if people don't like this idea, that's totally cool with me and I'll close the issue.

[bwoebi]

This sounds like a legitimate, though extremely edge case.
I've not once encountered a need for something like this.

But in this very special case you should be able to just implement your own Awaitable decorator, I'm not sure whether it belongs in the standard.

class RaceableAwaitable implements Awaitable {
    private $awaitable;
    private $racing = [];
    private $resolved = false;
    public function __construct(Awaitable $awaitable) {
        $this->awaitable = $awaitable;
        $awaitable->when(function($ex, $val) {
            $this->resolved = true;
            foreach ($this->racing as $deferred) {
                $deferred->resolve($this->awaitable);
            }
            unset($this->racing);
        });
    }
    public function race(Awaitable $awaitable) {
        if ($this->resolved) {
            return $this->awaitable;
        }
        $deferred = new Deferred;
        $hash = spl_object_hash($deferred);
        $this->racing[$hash] = $deferred;
        $awaitable->when(function() use ($awaitable, $hash); {
            if ($this->resolved) { return; }
            $deferred = $this->racing[$hash];
            unset($this->racing[$hash]);
            $deferred->resolve($awaitable);
        });
        return $deferred->awaitable();
    }
    public function when(callable $callback) { $this->awaitable->when($callback); } 
}

Then you can wrap this locally and do your calls against this custom race() method. Then you have exactly one when() call for all your races.

I do not think this case warrants adding an extra function to the fundamental Awaitable interface.
For special cases you can have special solutions.

[joshdifabio]

@bwoebi Thanks for taking the time to reply in detail. Having considered your point and thought about this for a while I think you're right; this is an edge case which can have its own solution and doesn't need to be baked into the spec.

[kelunik]

As it came up again in #38, we might want to reconsider don't care / unregister. I can see a problem with things like combinators / timeouts, where bound variables can't be GCed, because they're still bound via when(), even if the timeout has already been fired long ago.

I think it's really not a real world problem though, I can't remember having a promise where I repeatedly called combinators on, but it might be the case when using more advanced implementations such as observables for a streaming API, which are long-lived promises then.

/cc @martinschroeder

[bwoebi]

I still think it shouldn't be part of this standard. It is an edge case.

It should be trivial to add another method in the custom Observable implementation or to return a custom object allowing for cancellation (it doesn't violate LSP to return a value when the parent returns void).

I see no general use case for interoperability here. If you know that the Promise may take really long to resolve [IMHO Promises/Observables are the wrong tool to use for potentially infinitely active streams though. They should be used for anything which has an end and just intermittently informs about current progress/data arrived.], you also know the concrete implementation of the Promise too and may use cancellation if present.