Reduce memory consumption by avoiding circular reference from stream reader

[valga]

Stream reader has a circular reference to itself here: https://github.com/clue/php-http-proxy-react/blob/7258a76f492357874d0af1fe1f50a08209f61174/src/ProxyConnector.php#L137 so it can be cleaned only with GC. When a proxy is doing a lot of disconnects, it is turning into a nightmare.

[clue]

Thanks for filing this PR! Once the failing test is addressed, I'm not opposed to getting this 👍

[…] it can be cleaned only with GC. When a proxy is doing a lot of disconnects, it is turning into a nightmare.

Can you give some background on where this is an issue and how this problem can be reproduced? I'd like to ensure this change addresses the core of this issue and make sure we do not introduce any regressions in the future 👍

[valga]

Here is a test case:

$loop = React\EventLoop\Factory::create(); $proxy = new \Clue\React\HttpProxy\ProxyConnector('127.0.0.1:3128', new \React\Socket\Connector($loop)); $connector = new \React\Socket\Connector($loop, array( 'tcp' => $proxy, 'timeout' => 3.0, 'dns' => false, 'tls' => false, )); $loop->addPeriodicTimer(1, function () use ($connector){$connector->connect('google.com:80') ->then(function (\React\Socket\ConnectionInterface$stream){$stream->close(); printf("[+] %.3fMB\n", memory_get_usage() / 1024 / 1024)})->otherwise(function (\Exception$e){printf("[-] %.3fMB (%s)\n", memory_get_usage() / 1024 / 1024, $e->getMessage())})}); $loop->addTimer(60, function () use ($loop){printf( "%.3fMB => %d => %.3fMB\n", memory_get_usage() / 1024 / 1024, gc_collect_cycles(), memory_get_usage() / 1024 / 1024); $loop->stop()}); $loop->run();

Before fix:

[+] 1.425MB [+] 1.428MB [+] 1.432MB ... [+] 1.621MB [+] 1.624MB [+] 1.628MB 1.573MB => 1829 => 1.376MB 

After fix:

[+] 1.431MB [+] 1.431MB [+] 1.431MB ... [+] 1.431MB [+] 1.431MB [+] 1.431MB 1.368MB => 0 => 1.370MB 

So basically that circular reference eats about 3.5KB of memory for each successful connection. When you run a lot of clients with unstable proxy, it becomes the issue.

[valga]

I fixed the failing test.

By the way, if a proxy is not running, the test case leaks 20x faster:

[-] 1.260MB (Timed out after 3 seconds) [-] 1.335MB (Timed out after 3 seconds) [-] 1.409MB (Timed out after 3 seconds) ... [-] 5.242MB (Timed out after 3 seconds) [-] 5.317MB (Timed out after 3 seconds) [-] 5.391MB (Timed out after 3 seconds) 5.462MB => 23072 => 1.292MB 

But it seems it is a problem with the Socket component.

[valga]

Ok, I have done some research and found that leaks with timed out connections are produced by promise cancellation. Related PR: reactphp/socket#113.

The failing test is addressed and this PR is ready for final review.

[clue]

Thanks for looking into this and providing the elaborate description! 👍

IMO it makes sense to get this in as-is, as reducing the memory consumption by reducing the number of circular dependencies is certainly a good thing 👍

[clue]

@valga Thank you for looking into this and providing the test script here! I've just filed #23 which should fix this! 👍