Nullify connecting after canceling the timer

[choval]

#106 (comment)

Having a hard time reproducing this.

What basically happens is the connection closes on the database side, the connection is nullified before the idleTimer is cancelled.

mysql/src/Io/LazyConnection.php

Lines 62 to 69 in c420ca0

	$connection->on('close', function () {
	$this->connecting = null;
	if ($this->idleTimer !== null) {
	$this->loop->cancelTimer($this->idleTimer);
	$this->idleTimer = null;
	}
	});

My script somehow managed to run the idleTimer before the cancelTimer, and after connecting = null (yay for async).
...

My previous pr would actually trigger a new connection when trying to idle it... This just moves nullifying connecting AFTER canceling the idleTimer.

I can't seem to reproduce this, it was a one time fluke, which resulted in a fatal error and killed the script.

[clue]

@choval Thanks for spotting and your updated report! (original #106)

Unfortunately, the PR still does not contain any tests, so it's a bit unclear what issue is being resolved here. Also, a test would help us to ensure we do not introduce any regressions in the future.

The way I understand this piece of logic, your change should not change anything about the async behavior. The close handler will be executed synchronously and I don't see how this could trigger any async behavior. What do you think?

[clue]

Ping @choval, can you update the status here, does the problem persist or did you mange to resolve this in the meantime?

[choval]

sorry, the problem persists, got stuck with work, give me this week i'll see if i can code this scenario, if not i'll ping back~

[choval]

@clue I've managed to reproduce this, it takes a few hits.

PHP 7.2.16 (cli) (built: Mar 22 2019 08:49:28) ( NTS )
Running on mac

<?php
require('vendor/autoload.php');

$user = 'root';
$pass = 'x';
$host = 'localhost';
$schema = 'test';
$idle = 0.05;
$uri = "{$user}:{$pass}@{$host}/{$schema}?idle={$idle}";

$loop = React\EventLoop\Factory::create();
$factory = new React\MySQL\Factory($loop);

$con = $factory->createLazyConnection($uri);
$conk = $factory->createLazyConnection($uri); // Separate connection to kill the first connection

// Chase the query to kill it, simulates a server side connection close
$kill_timer = $loop->addPeriodicTimer($idle/2, function() use ($conk) {
  $conk->query('SHOW PROCESSLIST')
    ->then(function($cmd) use ($conk) { 
      foreach($cmd->resultRows as $row) {
        if($row['Info'] == 'SELECT * FROM test') {
          $pid = $row['Id'];
          $conk->query('KILL CONNECTION '.$pid);
        }
      }
    });
});

// The timer is 150% of the idle, so the idle method is triggered.
$stream;
$query_timer = $loop->addPeriodicTimer($idle*1.5, function() use ($loop, $con, &$stream) {
  if($stream) {
    $stream->close();
  }
  $stream = $con->queryStream('SELECT * FROM test');
  $con->query('SELECT * FROM test');
  echo '.'; // Hint it's running
});

$loop->addTimer( 10, function() use ($loop, $query_timer, $kill_timer) {
  $loop->cancelTimer($query_timer);
  $loop->cancelTimer($kill_timer);
});

$loop->run();

The close handler will be executed synchronously and I don't see how this could trigger any async behavior. What do you think?

The handler runs in two ticks, once for null assign, and then the if in a separate one, I believe due to the if.
(Hence, the chase in the test script).

[choval]

Please note that my PR doesn't fix it.

[clue]

@choval Thank you very much for the update, I can now reproduce this locally and I'm working on a fix for this 👍

Here's the updated gist to reproduce this 100%:

<?php

require('vendor/autoload.php');

$user = 'test';
$pass = 'test';
$host = 'localhost';
$schema = 'test';
$idle = 0.05;
$uri = "{$user}:{$pass}@{$host}/{$schema}?idle={$idle}";

$loop = React\EventLoop\Factory::create();
$factory = new React\MySQL\Factory($loop);

$con = $factory->createLazyConnection($uri);
$conk = $factory->createLazyConnection($uri); // Separate connection to kill the first connection

// Chase the query to kill it, simulates a server side connection close
$kill_timer = $loop->addPeriodicTimer($idle/2, function() use ($conk) {
    $conk->query('SHOW PROCESSLIST')
    ->then(function($cmd) use ($conk) {
        foreach($cmd->resultRows as $row) {
            if($row['Info'] == 'SELECT SLEEP(1)') {
                $pid = $row['Id'];
                $conk->query('KILL CONNECTION '.$pid);
            }
        }
    });
});

$stream = $con->queryStream('SELECT SLEEP(1)');
$stream->on('data', 'var_dump');
$stream->on('error', function (Exception $e) {
    echo 'stream error: ' . $e->getMessage() . PHP_EOL;
});
$stream->on('close', function () use ($loop, $kill_timer) {
    echo 'closed stream' . PHP_EOL;
    $loop->cancelTimer($kill_timer);
});

$loop->run();

[clue]

@choval Again thanks for reporting! I was able to reproduce this and have just filed a fix in #110

It also looks like there might be a similar race condition in clue/reactphp-redis#87 and clue/reactphp-sqlite#23, so I'll look into this in follow-up PRs as well 👍

[choval]

Thanks for all the awesome libraries! Im closing this PR as it doesn't fix anything :-)