Deprecate PingableConnection in favor of handling lost connection

[morozov]

Q	A
Type	improvement, deprecation
BC Break	no

1. The "ping" functionality in the DBAL is really questionable. The only way to check if the connection is alive is to execute a query. And even if one query succeeds or fails, it doesn't guarantee that another one will succeed or fail as well. Additionally, it's only supported by mysql-based drivers.
2. Instead, the wrapper connection can handle a ConnectionLost exception if it's reported by the driver. This means the behavior is happening automatically from now on.
3. Statement constructors are marked internal. It will allow to remove the dependency of the wrapper Statement on the wrapper Connection similarly to how it was done with the oci8 driver in Removed the OCI8Connection::getExecuteMode() method #3808. The new Connection::handle*() internal methods are temporary and will be moved to an ExceptionHandler after refactoring.

[healerz]

Hi @morozov
I have a question regarding this change. In our project, we use ping() to ensure the connection is not lost:

if (false === $this->ping($connection)) {
  $connection->close();
  $connection->connect();
}

Correct me if I'm wrong, but if I understand this change correctly, the difference is that the connection gets closed properly if the connection is lost. But it doesn't execute the query again, in such a case. So it only ensures that the next query succeeds again.
What is the alternative to ensure the connection is not lost due to a connection timeout, so we can safely execute our actual query?
Or is the solution to execute a dummy query and catch the ConnectionLost exception?
Even though I do agree with your reasoning above :)

[morozov]

Correct me if I'm wrong, but if I understand this change correctly, the difference is that the connection gets closed properly if the connection is lost. But it doesn't execute the query again, in such a case. So it only ensures that the next query succeeds again.

Sounds correct.

What is the alternative to ensure the connection is not lost due to a connection timeout, so we can safely execute our actual query?

There's no way to ensure that it's not lost. Even if the query you use to probe the connection succeeds, there's no guarantee that the real query will.

Or is the solution to execute a dummy query and catch the ConnectionLost exception?

See the above. The solution is to catch the ConnectionLost exception and handle it depending on the query. E.g. it is safe to re-execute a SELECT but isn't safe to re-execute an INSERT w/o first checking the current state of the database (the connection may fail after the row has been inserted).

[healerz]

@morozov thx for your reply. Makes all sense.

[flmommens]

I catch the exception and immediately retry my SELECT query but get the same exception again.

[morozov]

@flmommens please file a separate issue and provide all the details of the failure. The basic case is covered with this test:

dbal/tests/Doctrine/Tests/DBAL/Functional/Connection/ConnectionLostTest.php

Lines 31 to 51 in 0a5bac1

	public function testConnectionLost(): void
	{
	$this->connection->query('SET SESSION wait_timeout=1');
	sleep(2);
	$query = $this->connection->getDatabasePlatform()
	->getDummySelectSQL();
	try {
	// in addition to the error, PHP 7.3 will generate a warning that needs to be
	// suppressed in order to not let PHPUnit handle it before the actual error
	@$this->connection->executeQuery($query);
	} catch (ConnectionLost $e) {
	self::assertEquals(1, $this->connection->fetchOne($query));
	return;
	}
	self::fail('The connection should have lost');
	}

[vudaltsov]

@morozov, why don't you check instanceof ConnectionException?

[vudaltsov]

I think, I got it. ConnectionException is mainly thrown when credentials are invalid, so reconnecting will not help.