Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Enhancement: expose ping() on pingable-connections. #414

Merged
merged 5 commits into from
Nov 15, 2013
Merged

Enhancement: expose ping() on pingable-connections. #414

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7d77a3f
Enhancement: expose ping() on pingable-connections.
till Nov 14, 2013
dc6bad0
Bugfix: coding standard & missing assertion
till Nov 14, 2013
11d160b
Enhancement: implement ping() for all drivers
till Nov 15, 2013
bffed61
Enhancement: CS fixes
till Nov 15, 2013
99657f7
Enhancement: use connect() to simplify the test
till Nov 15, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions lib/Doctrine/DBAL/Connection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
use Doctrine\DBAL\Cache\QueryCacheProfile;
use Doctrine\DBAL\Cache\ArrayStatement;
use Doctrine\DBAL\Cache\CacheException;
use Doctrine\DBAL\Driver\PingableConnection;

/**
* A wrapper around a Doctrine\DBAL\Driver\Connection that adds features like
Expand Down Expand Up @@ -1488,4 +1489,30 @@ public function createQueryBuilder()
{
return new Query\QueryBuilder($this);
}

/**
* Ping the server!
*
* @return bool
*/
public function ping()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you implement the method, I would implement PingableConnection on this class too

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is the main connection and not connected (despite naming) with the real connections.

{
if ( ! $this->_isConnected) {
return false; // Don't connect if not done yet. It will be lazy on first use
}

if ($this->_conn instanceof PingableConnection) {
return $this->_conn->ping();
}

try {
$this->query($this->_platform->getDummySelectSQL());
return true;
} catch (DBALException $e) {
// As the underlying connection is unset, the next query will connect again thanks to the lazyness
$this->close();
}

return false;
}
}
13 changes: 12 additions & 1 deletion lib/Doctrine/DBAL/Driver/Mysqli/MysqliConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,12 @@
namespace Doctrine\DBAL\Driver\Mysqli;

use Doctrine\DBAL\Driver\Connection as Connection;
use \Doctrine\DBAL\Driver\PingableConnection;

/**
* @author Kim Hemsø Rasmussen <kimhemsoe@gmail.com>
*/
class MysqliConnection implements Connection
class MysqliConnection implements Connection, PingableConnection
{
/**
* @var \mysqli
Expand Down Expand Up @@ -207,4 +208,14 @@ private function setDriverOptions(array $driverOptions = array())
);
}
}

/**
* Pings the server and re-connects when `mysqli.reconnect = 1`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when mysqli.reconnect = 0 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof http://docs.php.net/manual/en/mysqli.configuration.php#ini.mysqli.reconnect

Doesn't reconnect.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this means that the effect of calling $connection->ping() and then doing another query will be different for MySQLi without reconnect than for other drivers (not implementing PingableConnection): it will fail as the connection will not be reset to connect again.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof true or false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@till But the implementation in Doctrine\DBAL\Connection for non-pingable connections will always allow you to do the next query, as it will connect again to the server.

return false seems to have a different meaning between the MySQLi implementation (return false when it failed to reconnect) and the DBAL implementation (return false when it is not connected at the time of the ping, without any relation to the behavior for next queries, which will always be connected)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@till @stof Personally, I would throw an exception when there was an error while reconnecting. If automatic reconnecting is disabled in Mysqli it should return false as @stof mentioned, to be consistent in the API. Errors in the physical reconnect should throw an exception IMO as I would expect that from the other driver implementations aswell.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deeky666 the Doctrine\DBAL\Connection implementation cannot throw an exception if it fails to connect, because it does not try. If it is not connected anymore, it closes the existing (timed out) connection. The actual reconnection will only happen on the next query execution, through the lazy connecting.
An exception will occur only the next time you do a query if it cannot connect (for instance if the server is down instead of being only a connection timeout), exactly like for an initial connection (because it is an initial connection).

So the issue here is that we have a different behavior between 2 cases:

  • underlying connection is not a PingableConnection: return true if we are currently connected. Return false if the connection timed out or if we are not connected yet. This does not indicate that you should stop using the connection, as next query will connect lazily.
  • underlying connection is a PingableConnection: the behavior depends on the implementation. And nothing guarantees that the next query can be done.

Here are my suggestions:

  • change Doctrine\DBAL\Connection::ping to get rid of the return value (we don't actually care whether we are connected or the next query will do a reconnection. All we want is making DBAL close itself in case of timeout to restart the lazy connection later).
  • document the PingableConnection so that it returns false if the connection timed out and cannot reconnect itself
  • if the PingableConnection return false, close the connection in Doctrine\DBAL\Connection to reconnect later the hard way (like for connections which are not pingable.

what do you think about it ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't returning boolean false or null (void) but never boolean true an inconsistent return behaviour? I would expect a method that indicates to return a boolean value to return both states. But I agree that \Doctrine\DBAL\Connection should not return any value as this is not necessary as you mentioned.
I think that the PingableConnection::ping() method should have a more descriptive DocBlock to make clear what to return in which case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deeky666 In my proposal, I don't have false|null. The DBAL facade always return void, and the PingableConnection for underlying connection always return a boolean

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stof Alright then I'll agree with you.

*
* @return bool
*/
public function ping()
{
return $this->_conn->ping();
}
}
7 changes: 7 additions & 0 deletions lib/Doctrine/DBAL/Driver/PingableConnection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing license

namespace Doctrine\DBAL\Driver;

interface PingableConnection
{
public function ping();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing phpdoc explaining what the implementation should do

}
11 changes: 11 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,4 +216,15 @@ public function testQuote()
{
$this->assertEquals($this->_conn->quote("foo", Type::STRING), $this->_conn->quote("foo", \PDO::PARAM_STR));
}

public function testPingDoesNotTriggerConnect()
{
$this->assertFalse($this->_conn->ping());
}

public function testPingReturnsTrueWhenConnectionIsPingedOrOpen()
{
$this->_conn->executeQuery($this->_conn->getDatabasePlatform()->getDummySelectSQL());
$this->assertTrue($this->_conn->ping());
}
}
6 changes: 6 additions & 0 deletions tests/Doctrine/Tests/DBAL/Functional/Mysqli/ConnectionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,12 @@ public function testUnsupportedDriverOption()
$this->getConnection(array('hello' => 'world')); // use local infile
}

public function testPing()
{
$conn = $this->getConnection(array());
$this->assertTrue($conn->ping());
}

private function getConnection(array $driverOptions)
{
return new \Doctrine\DBAL\Driver\Mysqli\MysqliConnection(
Expand Down