Memory leak

[dmarkic]

Hello,

When command is finished, it stays in memory as evenement/evenement events are still registered with command and so command is never destructed.

Version: 5.7.x PHP Version: 7.4.33

A simple reproduction script:

$loop = \React\EventLoop\Loop::get();
$factory = new \React\MySQL\Factory();
$connection = $factory->createLazyConnection('localhost');

$loop->addPeriodicTimer(
    1,
    function () use ($connection) {
        $connection->query('SELECT \'' . str_repeat('A', 102400) . '\'')->done(
            function () {
                echo " - query done\n";
            },
            function ($e) {
                echo " - query error: " . $e->getMessage() . "\n";
            }
        );
        echo " - Memory usage: " . memory_get_usage() . "\n";
    }
);

$loop->run();

Example output:

 - Memory usage: 3563304
 - query done
 - Memory usage: 4415176
 - query done
 - Memory usage: 4638312
 - query done
 - Memory usage: 4861448
 - query done
 - Memory usage: 5084584
 - query done
 - Memory usage: 5307720
 - query done
 - Memory usage: 5530856
 - query done

I've made few changes to Io\Parser.php to call $command->removeAllListeners() in methods:

• onError
• onResultDone
• onSuccess
• onClose

It has solved the memory issue.

I think that's a major issue, should I create a simple PR where removeAllListeners() is called on command when it's finished?

Kind regards, Dejan Markic

[SimonFrings]

Thank you @dmarkic for bringing this up :+1:

This is definitely an interesting one. We're currently not aware of any memory leaks in this project, but your input above seems like you may have found one. In order to make sure this is actually a memory leak, we need some additional tests for the event listeners (if they get removed successfully) to support this thesis. Is this something you can look into?

[dmarkic]

Hello,

Sorry for late reply, just came back from vacation.

You mean tests with this removeAllListeners() on commands?

[kodmanyagha]

This problem still exist. I changed Io/Parser.php file like that but problem didn't solve:

    private function onError(\Exception $error)
    {
        $this->rsState = self::RS_STATE_HEADER;
        $this->resultFields = [];

        // reject current command with error if we're currently executing any commands
        // ignore unsolicited server error in case we're not executing any commands (connection will be dropped)
        if ($this->currCommand !== null) {
            $command = $this->currCommand;
            $this->currCommand = null;

            $command->emit('error', [$error]);
            $command->removeAllListeners();
        }
    }

    protected function onResultDone()
    {
        $command = $this->currCommand;
        $this->currCommand = null;

        $command->resultFields = $this->resultFields;
        $command->emit('end');
        $command->removeAllListeners();

        $this->rsState = self::RS_STATE_HEADER;
        $this->resultFields = [];
    }

    protected function onSuccess()
    {
        $command = $this->currCommand;
        $this->currCommand = null;

        if ($command instanceof QueryCommand) {
            $command->affectedRows = $this->affectedRows;
            $command->insertId = $this->insertId;
            $command->warningCount = $this->warningCount;
            $command->message = $this->message;
        }
        $command->emit('success');
        $command->removeAllListeners();
    }

    public function onClose()
    {
        if ($this->currCommand !== null) {
            $command = $this->currCommand;
            $this->currCommand = null;

            if ($command instanceof QuitCommand) {
                $command->emit('success');
            } else {
                $command->emit('error', [new \RuntimeException(
                    'Connection closing (ECONNABORTED)',
                    \defined('SOCKET_ECONNABORTED') ? \SOCKET_ECONNABORTED : 103
                )]);
            }
            $command->removeAllListeners();
        }
    }

Try with that:

        $loop->addPeriodicTimer(
            3,
            function () use ($logger) {
                //gc_collect_cycles();
                $logger->info(">> Memory usage: " . number_format(memory_get_usage() / 1024 / 1024, 4, '.', '') . " MB");
            }
        );

...

        for ($i = 0; $i < 10; $i++) {
            $this->loop->addPeriodicTimer(
                0.1,
                function () {
                    // TODO This is causing memory leak. Solve this.
                    $this->pool
                        ->query('SELECT \'' . str_repeat('A', 102400) . '\'')
                        ->then(function (QueryResult $queryResult) {
                            //$this->logger->info(">> Query done");
                        })
                        ->catch(function (Throwable $throwable) {
                            $this->logger->error(">> Query error: " . $throwable->getMessage());
                        });
                }
            );
        }

[kodmanyagha]

Probably I found the problem. In React\MySQL\Commands\QueryCommand class we must add resultFields field. Otherwise PHP8.2 will throw that error: "Deprecated: Creation of dynamic property...". Then our error handler trying to print everything to log file and tataa!! The huge string (AAAAAAA...) and stack trace storing in somewhere. If you add this property to QueryCommand then everything works. There isn't any memory leak in ReactPHP but in our framework is trying to handle deprecated error.

Edit: It seems this problem occurs in PHP8.2 only. In PHP8.1 it doesn't increasing memory usage.

[2023-10-30T16:37:54.177030+00:00] default.INFO: Memory usage: 92.16 MB [] []
[2023-10-30T16:37:59.177776+00:00] default.INFO: Memory usage: 20.73 MB [] []
[2023-10-30T16:37:59.505713+00:00] default.INFO: 10000 queries executed. [] []
[2023-10-30T16:38:04.181589+00:00] default.INFO: Memory usage: 28.44 MB [] []
[2023-10-30T16:38:09.182401+00:00] default.INFO: Memory usage: 49.68 MB [] []
[2023-10-30T16:38:09.783651+00:00] default.INFO: 10000 queries executed. [] []
[2023-10-30T16:38:14.182619+00:00] default.INFO: Memory usage: 66.93 MB [] []
[2023-10-30T16:38:19.182842+00:00] default.INFO: Memory usage: 83.14 MB [] []
[2023-10-30T16:38:20.030727+00:00] default.INFO: 10000 queries executed. [] []
[2023-10-30T16:38:24.183206+00:00] default.INFO: Memory usage: 92.16 MB [] []
[2023-10-30T16:38:29.183843+00:00] default.INFO: Memory usage: 28.44 MB [] []
[2023-10-30T16:38:30.299647+00:00] default.INFO: 10000 queries executed. [] []
[2023-10-30T16:38:34.188066+00:00] default.INFO: Memory usage: 49.68 MB [] []
[2023-10-30T16:38:39.188306+00:00] default.INFO: Memory usage: 69.84 MB [] []
[2023-10-30T16:38:40.555126+00:00] default.INFO: 10000 queries executed. [] []
[2023-10-30T16:38:44.193994+00:00] default.INFO: Memory usage: 83.14 MB [] []
[2023-10-30T16:38:49.198063+00:00] default.INFO: Memory usage: 92.16 MB [] []
[2023-10-30T16:38:50.794827+00:00] default.INFO: 10000 queries executed. [] []
[2023-10-30T16:38:54.199078+00:00] default.INFO: Memory usage: 28.44 MB [] []

As you can see memory is wandering between 92MB and 28MB. Garbage collector is cleaning it up. It is fine but if you handle PHP8.2 issues than the life will be better I think :) Thanks.

[SimonFrings]

@dmarkic We need a test that expects all listeners to be removed at the end, which would fail if this isn't currently the case. Depending on the outcome of this test we would then have the proof (or not) if this is actually the case here.

@kodmanyagha Thanks for your input on this! This has already been fixed in https://github.com/friends-of-reactphp/mysql/pull/170 and was a oversight from https://github.com/friends-of-reactphp/mysql/pull/161. As I described in https://github.com/friends-of-reactphp/mysql/issues/175#issuecomment-1785311945, we're currently working on a MySQL connection pool feature and plan this for the future v0.7.0. The changes from #170 are part of the v0.6.0 which we're planning to release somewhere around next week. You should then be able to upgrade to the new version and shouldn't see the deprecation notices anymore.

We currently don't have the confirmation that there's actually a memory leak in this ticket, so I will close this for now. We can reopen this if we have a test to confirm the leak and then start working on a fix.