1
\$\begingroup\$

I am trying to create class with connect to mysql database. And if there is max number of connections I want to wait and try it again. I figured out, how it can works, but I am not sure, if its the right way of doing it. So, my code looks like this:

<?php

class DbConnect {

  private $attemps = 1;
  private $errorCode;
  private $maxAttemps = 10;
  private $pdo;
  private $dsn = "mysql:host=localhost;dbname=...;charset=utf8";
  private $options = [
    PDO::ATTR_EMULATE_PREPARES   => false, // turn off emulation mode for "real" prepared statements
    PDO::ATTR_ERRMODE            => PDO::ERRMODE_EXCEPTION, //turn on errors in the form of exceptions
    /*PDO::ATTR_DEFAULT_FETCH_MODE => PDO::FETCH_ASSOC,*/ //make the default fetch be an associative array
  ];

  public function __construct(){
    $this->tryConnect();
    while ($this->errorCode == 1040 && $this->attemps <= $this->maxAttemps) {
      usleep(pow(2, $this->attemps)*10000);
      $this->tryConnect();
    }

  }


  protected function tryConnect(){
    try {
      $this->pdo = new PDO($this->dsn, "root", "", $this->options);
    } catch (Exception $e) {
      error_log($e->getMessage());
      $this->errorCode = $e->getCode();
      $this->attemps++;
    }
  }



}

?>

Is there anything i should change? Thank you very much.

\$\endgroup\$
10
  • 1
    \$\begingroup\$ The class has no methods to work with the open connection. And if the connection fails 10 times it claims to have succeeded but actually it is not connected. So I would say the code does not work correctly and the question is probably missing some context... \$\endgroup\$
    – slepic
    Nov 22, 2020 at 6:18
  • 1
    \$\begingroup\$ Anyway, if connection pool is full you probably don't want php request processes to stack up and make even larger queue for open db connections. The 10 attempts can take up to cca 20 seconds, if client closes the connection, the server does not, and so the php process keeps waiting for db although there is nobody waiting for data any more. This might be a good thing in a background task that never runs multiple times simultaneously. But not for a client request handler. At least not in general. \$\endgroup\$
    – slepic
    Nov 22, 2020 at 6:27
  • \$\begingroup\$ I can add public property which will contain if its connected or not and cut connection with set new Db object to null. I understand what you mean, but I have no idea how to make queue on server and then tell clients to connect, because its normall web hosting.. \$\endgroup\$ Nov 22, 2020 at 9:40
  • 1
    \$\begingroup\$ You better just inform the client that you cannot fulfill their request (with a 5xx response status) and have them retry later at their own will. Most users probably won't wait for the page to load after few seconds of nothing happening. They will just hit f5 or navigate away not knowing what's wrong, probably thinking the page is broken and never coming back. \$\endgroup\$
    – slepic
    Nov 22, 2020 at 9:56
  • \$\begingroup\$ You think 509 Bandwidth Limit Exceeded (Apache)? \$\endgroup\$ Nov 22, 2020 at 10:01

2 Answers 2

1
\$\begingroup\$

You code has several problems. Specificaly it breaks at least two of the SOLID principles, namely single responsibility and dependency inversion principles.

Single responsibility is violated because the class is responsible for:

  • connecting to db
  • retrying connection
  • whatever other methods provided by the class you did not show us

Dependency inversion principle is violated because the class uses hardcoded values (although stored in instance properties).

Let me first separate out the first responsibility. We don't really need OOP for that, so let me do it in FP style.

function createMyPdo(): \PDO
{
  $dsn = ...;
  $user = ...;
  $password = ...;
  $options = ...;
  return new \PDO($dsn, $user, $password, $options);
}

This function now opens a connection with hardcoded credentials. Although the credentials are hardocded and we would better pull those values off of maybe environment variables. It at least has just one responsibility and that is to open the specific database connection.

Now lets implement the retry logic as a separate thing, that only depends on something that creates PDO instance.

function createPdoWithRetry(callable $factory, int $attempts): \PDO
{
  $attemptsMade = 0;
  while ($attempts > $attemptsMade) {
    try {
      return $factory();
    } catch (\PDOException $e) {
      if ($e->getCode() === 1040) {
        \usleep(\pow(2, $attempsMade) * 10000);
        ++$attemptsMade;
      } else {
        throw $e;
      }
    }
  }
  throw new \RuntimeException("All $attemptsMade retry attempts exhausted.");
}

Now I can easily open the connection with retry logic

$pdo = createPdoWithRetry(fn () => createMyPdo(), 20);

And I can also open a different connection using the same thing:

$pdo2 = createPdoWithRetry(fn () => createOtherPdo(), 20);

This approach still has some caveats:

  • where createMyPdo could only throw PDOException, createPdoWithRetry can also throw a generic RuntimeException.
  • the type of the $factory parameter has a very vague type, that we have to describe in a docblock and cannot be enforced by PHP runtime.
  • createPdoWithRetry knows error code 1040 which is mysql specific, but the knowledge that mysql is used is only known to the createMyPdo function.

Let's make it better by going back to OOP style and introducing an interface for the connection factory, including specific exceptions to be thrown (which will abstract away the 1040 speific mysql error code).

class DatabaseConnectionException extends \Exception
{
}

class TooManyConnectionsException extends DatabaseConnectionException
{
}

interface DatabaseConnector
{
  /**
   * @return \PDO
   * @throws DatabaseConnectionException
   * @throws TooManyConnectionsException
   */
  public function createPdo(): \PDO;
}

As you can see I added some @throws annotations to:

  • inform implementors of the interface which exception they can throw
  • informs consumers of the interface which exceptions they can catch

Although PHP cannot enforce the actual type of exceptions thrown from the implementations, documenting the exceptations makes it less likely that someone will implement it wrong.

Now lets make an implementaton for our specific connection:

class MyDatabaseConnector implements DatabaseConnector
{
  public function createPdo(): \PDO
  {
    $dsn = ...;
    $user = ...;
    $password = ...;
    $options = ...;
    try {
      return new \PDO($dsn, $user, $password, $options);
    } catch (\PDOException $e) {
      if ($e->getCode() === 1040) {
        throw new TooManyConnectionsException($e->getMessage(), (int) $e->getCode(), $e);
      } else {
        throw new DatabaseConnectionException($e->getMessage(), (int) $e->getCode(), $e);
      }
    }
  }
}

Now maybe you see why the credentials should not be hardocded, because the same code would work for any credentials. Hardcoding the credentials, we are doomed to repeat some code if we want to open a different connection.

But I leave that up to you to find your way to the dependency inversion principle. Basically constructor of a class should accept things that the class instances need to know, rather then having the instances deciding on their own what those values should be (ie. pass the credentials through constructor, rather then hardocing the credentials withing the class).

Anyway, to implement the retry logic we will use the same interface:

class RetryingDatabaseConnector implements DatabaseConnector
{
  private DatabaseConnector $factory;
  private RetryDelayStrategy $delayStrategy;
  private int $maxAttempts;

  public function __construct(DatabaseConnector $factory, RetryDelayStrategy $delayStrategy, int $maxAttempts)
  {
    $this->factory = $factory;
    $this->delayStrategy = $delayStrategy;
    $this->maxAttempts = $maxAttempts;
  }

  public function createPdo(): \PDO
  {
    $attemptsMade = 0;
    while ($this->maxAttempts > $attemptsMade) {
      try {
        return $this->factory->createPdo();
      } catch (TooManyConnectionsException $e) {
          ++$attemptsMade;
          $this->delayStrategy->wait($attemptsMade)
      }
    }
    throw new TooManyConnectionsException("All $attemptsMade retry attempts exhausted.");
  }
}

As you can see I have also extracted the delay logic to a separate interface, because exponentialy growing gaps between attempts is just one of many possible strategies. And again, you don't want to implement a new connector that would repeat a lot of the logic just to change to delay strategy.

interface RetryDelayStrategy
{
  public function wait(int $attemptsMade): void;
}

class ExponentialRetryDelayStrategy implements RetryDelayStrategy
{
  private int $coefficient;
  private int $base;

  public function __construct(int $coefficient, int $base = 2)
  {
    $this->coefficient = $coefficient;
    $this->base = $base;
  }

  public function wait(int $attemptsMade): void
  {
    \usleep(\pow($this->base, $attempsMade) * $this->coefficient);
  }
}

And finaly some consumer code. A controller that opens a database connection (to do something with it) and gives a user friendly messages if the connection fails and a more specific one if the connection fails specificaly because of too many open connections.

class MyController extends BaseController
{
  private DatabaseConnector $connector;

  public function __construct(DatabaseConnector $connector)
  {
    $this->connector = $connector;
  }

  public function myControllerAction(Request $request): Response
  {
    try {
      $pdo = $this->connector->createPdo();
    } catch (TooManyConnectionsException $e) {
      return $this->send(503, "Too many database connections.");
    } catch (DatabaseConnectionException $e) {
      return $this->send(503, "Database unavailable.");
    }

    // do something with PDO here

    return $this->send(200, "All done");
  }
}

Now, you see the controller just depends on a DatabaseConnector. It doesn't really care if and how many times the connector will retry. It just cares that it can fail in a specific (TooManyConnectionsException) or an further unspecified (DatabaseConnectionException) way or, if it succeeds, it returns a PDO instance. And it will work no matter what connector you pass to it, as long as the interface is implemented correctly. And so you can change the implementation without touching the controller code. Again, thanks to single responsibility a dependency inversion principles.

No matter which is used in the DI setup

$myConnector = new MyDatabaseConnector();
// or maybe if we pull it to envs
// $myConnector = new MyDatabaseConnector($_ENV['DB_DSN'], $_ENV['DB_USER'], $_ENV['DB_PASS']);
$connector = $myConnector;

or

$delayStrategy = new ExponentialRetryDelayStrategy(10000, 2);
$retryingConnector = new RetryingDatabaseConnector($myConnector, $delayStrategy, 20);
$connector = $retryingConnector;

the controller code stays untouched

$controller = new MyController($connector);
...
$controller->run();

Also notice, how I avoided setting an errorCode and checking it in the controller. Instead we just throw exceptions. This forces the caller to actually handle the error or let it bubble up the stack and eventually stop execution of the program. This is the prefered behaviour because if you would forget to check the error code, it really does not make sense to try to send sql queries over a "not opened connection".

Btw, if you are interested in design patterns, you can notice that:

  • DatabaseConnector and its implementations are a factory method pattern
  • RetryingDatabaseConnector and the interface follow the decorator pattern
  • RetryDelayStrategy and its implementations are the strategy of the strategy design pattern and RetryingDatabaseConnector is the strategy consumer
\$\endgroup\$
3
  • \$\begingroup\$ Thank you very much for your loooong reply. But now, I dont really know what to do, because this seems like whole new level of programming. I went throught interfaces and extending classes with classes etc. in past, but this whole logic is something.. I dont know how to say it. It seems like its from big practice, which I dont have and dont have any options to get some, from real world.. \$\endgroup\$ Nov 23, 2020 at 10:39
  • \$\begingroup\$ @TomášKretek yeah, I thought it might too high level for you. take your time, read it several times and try to understand the individual parts... The big message is that you should split your code into reusable pieces with single responsibilities. Don't try to stuck everything in one class. The program flow comes in stages (gather config, create objects, work with objects), don't be shy to represent each stage as a separate interface/class. Think what a class needs to do to fulfill its purpose, then outsource them - ask for them via constructor, but let the instantiator decide what to pass in \$\endgroup\$
    – slepic
    Nov 23, 2020 at 11:05
  • \$\begingroup\$ And only ask for what you really need. If you ask for something with dozen of methods and only use one of them, then something is probably wrong. Your class there was doing a lot of stuff, probably has methods that you didn't include. But what consumers really want is to just get a PDO instance so they can work with it. That's the reason why you don't see any wrapper around the PDO instance in my implementation, it simply is not necesary, becase the other stages of the flow have already been resolved elsewhere... \$\endgroup\$
    – slepic
    Nov 23, 2020 at 11:06
1
\$\begingroup\$

"Too many connections"...

  • Use only one connection for the entire program. (There are rare exceptions.)
  • Disconnect when finished.
  • Decrease wait_timeout -- this will do the disconnect in case you forget to do it.
  • Make sure no other system (eg, the web server) is causing excessive number of connections)
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.