-
-
Notifications
You must be signed in to change notification settings - Fork 145
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
Memory leak when using persistent connections #514
Comments
Hey @mmoreram, just tried to see if keep alive indeed leaks memory due to that. And I can't prove that it does. Modified example 51 into this: <?php
use React\EventLoop\Loop;
$reportMemory = static function () {
$rawInt = memory_get_usage(false);
$rawExt = memory_get_usage(true);
echo date('r'), ' Internal: ', ($rawInt / 1024), 'KB External: ', ($rawExt / 1024), 'KB (Raw Internal: ', $rawInt, ' Raw External: ', $rawExt, ')', PHP_EOL;
};
$reportMemory();
echo 'Loading autoloader', PHP_EOL;
require __DIR__ . '/../vendor/autoload.php';
$reportMemory();
echo 'Creating HTTP Server', PHP_EOL;
$http = new React\Http\HttpServer(function (Psr\Http\Message\ServerRequestInterface $request) {
return React\Http\Message\Response::plaintext(
"Hello World!\n"
);
});
$reportMemory();
echo 'Creating Socket Server', PHP_EOL;
$socket = new React\Socket\SocketServer(isset($argv[1]) ? $argv[1] : '0.0.0.0:0');
$reportMemory();
echo 'Listening on socket', PHP_EOL;
$http->listen($socket);
Loop::addPeriodicTimer(60, $reportMemory);
echo 'Listening on ' . str_replace('tcp:', 'http:', $socket->getAddress()) . PHP_EOL;
$reportMemory();
echo 'Starting Event Loop', PHP_EOL; And then ran ab -k -c 100 -n 100000000 http://localhost:42353/ It came back after just over an hour with this: This is the output from the server. As you can see it stays stable at 3.9MB of memory used by PHP and 6MB allocated with the system throughout the test. After completed it dropped back to just over 2MB. I would love to have more information on what makes this a memory leak for you.
|
While having a chat with @SimonFrings this morning, we realised I wasn't using a promise in the test. After adjusting the test we confirmed the reported issue very clearly:
Currently working on adding unit tests to confirm the clean up of those close calls. |
This changeset resolves a small memory leak that causes roughly 1KB per connection tops. Which isn't a big issue but will make memory fluctuate more. The changeset doesn't introduce any performance degradation. Resolves: reactphp#514 Builds on top of: reactphp#405, reactphp#467, and many others
This changeset resolves a small memory leak that causes roughly 1KB per connection tops. Which isn't a big issue but will make memory fluctuate more. The changeset doesn't introduce any performance degradation. Resolves: reactphp#514 Builds on top of: reactphp#405, reactphp#467, and many others
This changeset resolves a small memory leak that causes roughly 1KB per connection tops. Which isn't a big issue but will make memory fluctuate more. The changeset doesn't introduce any performance degradation. Resolves: reactphp#514 Builds on top of: reactphp#405, reactphp#467, and many others
This changeset resolves a small memory leak that causes roughly 1KB per connection tops. Which isn't a big issue but will make memory fluctuate more. The changeset doesn't introduce any performance degradation. Resolves: reactphp#514 Builds on top of: reactphp#405, reactphp#467, and many others
@mmoreram Thanks for bringing this up, #515 by @WyriHaximus is now merged and should fix the reported issue. I also changed the title of this ticket to better match the described topic 👍 If you'd like to support this development, consider sponsoring ReactPHP! ❤️ Thank you! |
When using persistent connections, this code makes me think that could cause problems... This happens inside the
handleRequest
in StreamingServer file. If we use persistent connections, we are adding one close event per request, causing all responses being referenced, even when it is returned properly (not allowing GC make its work).The text was updated successfully, but these errors were encountered: