Improve memory consumption for cancelled connection attempts

[clue]

While debugging some very odd memory issues in a live application, I noticed that this component shows some unexpected memory consumption and memory would not immediately be freed as expected when a connection attempt is cancelled. Let's not call this a "memory leak", because memory was eventually freed, but this clearly caused some unexpected and significant memory growth.

One of the core issues has been located and addressed via reactphp/event-loop#164, but even with that patch applied, cancelling a pending connection attempt behaved a bit unexpected.

I've used the following script to demonstrate unreasonable memory growth:

<?phpuseReact\EventLoop\Factory; useReact\Socket\Connector; require__DIR__ . '/../vendor/autoload.php'; $loop = Factory::create(); $connector = newConnector($loop, array('timeout' => false)); $loop->addPeriodicTimer(0.001, function () use ($connector){$promise = $connector->connect('192.168.2.1:8080'); $promise->cancel()}); $loop->addPeriodicTimer(1.0, function (){echomemory_get_usage() . PHP_EOL}); $loop->run();

Initially this peaked at around 320 MB on my system. After applying the referenced patch, this went down significantly and fluctuated somewhere between 2 MB and 12 MB. After applying this patch, this script reports a constant memory consumption of around 1.2 MB.

This implementation includes some of the ideas discussed in reactphp/promise-timer#32, reactphp/promise#46 and #113. Eventually, we should look into providing a way to address this within our promise implementation.

My vote would to be get this in here now as it addresses a relevant memory issue and eventually address this in the upstream component (at which point this changeset also does no harm).

[kelunik]

How does this change anything in the memory consumption?

[andig]

I was wondering the same- can we release these if not passed by reference?

[clue]

@kelunik, @andig Have you seen the above description and the referenced tickets?

Creating a new Exception one line below will store a reference to the stack trace including the full call stack with all arguments passed. Because the $resolver and $canceller are always invoked with three callable arguments, these get implicitly stored as part of this stack. Because each of these arguments is a callable that is implicitly bound to the promise, this hence creates a cyclic reference. Note that these parameters are part of the call stack because they're passed at call time, despite not being used anywhere within this callback.

By explicitly assigning these arguments with null values, we can ensure that these null values end up in the call stack instead, effectively preventing these cyclic references. This allows PHP to rely on its refcount to immediate free these variables without having to rely on cyclic garbage collection kicking in at a later time after accumulating some memory.

As documented above, I would consider to be a temporary work around here. We should eventually look into providing a more permanent solution within react/promise.

To be clear, I very much appreciate this discussion. This PR exists as one possible option to avoid unneeded memory allocation and serves as a way to discuss possible options and how these should be addressed in the future. Any input is welcome 👍

[kelunik]

Thanks for the write-up, that makes it clear!

[andig]

Thanks for the write-up, that makes it clear!

Same here. It might still not hurt to add a code comment as to why the callbacks are nulled. It wasn't obvious to me although I had tried to digest the linked PRs/ issues.

[clue]

@andig Agreed, I think these changes make perfect sense for now, but this will be reverted as part of #161 once these issues have been resolved in the upstream components