laravel / framework

The Laravel Framework.
https://laravel.com
MIT License
32.25k stars 10.92k forks source link

PDO Too many connections when running test suite #18471

Closed ryrobbo closed 6 years ago

ryrobbo commented 7 years ago

Description:

I have a test suite which now has 140 tests. Each of my tests uses the DatabaseMigrations trait along with a dedicated database solely for tests to run against.

When PHP Unit gets to the last test it fails with the following error:

Caused by
Doctrine\DBAL\Driver\PDOException: SQLSTATE[08004] [1040] Too many connections

I can run this test individually and it passes fine.

After finding this issue (https://github.com/laravel/framework/issues/10619#issuecomment-151075567) I tried the following but the problem still persists:

$this->beforeApplicationDestroyed(function () {
    $this->artisan('migrate:rollback');

    foreach ($this->app->make('db')->getConnections() as $connection) {
        $connection->disconnect();
    }
});

Steps To Reproduce:

N/A

Kyslik commented 7 years ago

Its problem with max_connections.

Naive solution is to increase max_connections using (run query)

set global max_connections = 200;

To check current max_connections

show variables like "max_connections";

I tested max_connections = 10 and run 60 tests and it failed with

Caused by
PDOException: SQLSTATE[HY000] [1040] Too many connections
sisve commented 7 years ago

I believe the problem is that the connection should be reused (or a few of them pooled), not that the database has a too low max_connections value.

Kyslik commented 7 years ago

What exactly do you mean reused? Connection is closed after each test, and until its closed new one opens up...

I tested this using Innotop on my local machine (installed from homebrew), I have 69 test total, and each test class uses DatabaseMigrations trait and Innotop showed 69 connections. I guess PHPUnit is fast so connections are not closed ASAP.

I found this post about this issue.

public function tearDown()
{
    $this->beforeApplicationDestroyed(function () {
        DB::disconnect();
    });

    parent::tearDown();
}

After that max_connection stats were ±3.

patinthehat commented 7 years ago

I had a similar problem not too long ago in Lumen, and solved it with:

    public function tearDown()
    {
        \DB::connection()->setPdo(null);
    }
arubacao commented 7 years ago

I can also confirm this issue. Manually closing database connections solved the issue. Contrary to @ryrobbo i hook into the DatabaseTransactions Traits beforeApplicationDestroyed method. Simply override the setUpTraits method in TestCase. Thereby you bypass overkill solutions like @patinthehat or @Kyslik since most likely not every test interacts with the db.

    protected function setUpTraits()
    {
        parent::setUpTraits();

        $uses = array_flip(class_uses_recursive(static::class));

        if (isset($uses[DatabaseTransactions::class])) {
            $database = $this->app->make('db');
            $this->beforeApplicationDestroyed(function () use ($database) {
                foreach ($this->connectionsToTransact() as $name) {
                    $database->connection($name)->disconnect();
                }
            });
        }
    }
ryrobbo commented 7 years ago

@arubacao

Do you use the DatabaseMigration's trait in your tests?

Your solution looks like it's only applicable if you're using the DatabaseTransations trait.

arubacao commented 7 years ago

@ryrobbo yes, that is correct. I just tried this as well (without the code from the previous post):

if (isset($uses[DatabaseMigrations::class])) {
            $this->beforeApplicationDestroyed(function () {
                $this->app->make('db')->connection('mysql')->disconnect();
            });
        }

And it worked for me. I have to be explicit here, because i have a mongodb connection open which doesn't cause any problems.

ryrobbo commented 7 years ago

@arubacao

I have modified my TestCase class as follows:

abstract class TestCase extends BaseTestCase
{
    use CreatesApplication;

    protected function setUpTraits()
    {
        parent::setUpTraits(); // TODO: Change the autogenerated stub

        $uses = array_flip(class_uses_recursive(static::class));

        if (isset($uses[DatabaseMigrations::class])) {
            $this->beforeApplicationDestroyed(function () {
                $this->app->make('db')->connection('mysql_test')->disconnect();
            });
        }
    }
}

Note: I am using an database connection called "mysql_test" to run my migrations during tests.

But I am still getting the too many connections error.

coquer commented 7 years ago

The best way we find to fix this issue was to move the test to sqlite driver:

'testing' => [
     'driver' => 'sqlite',
    'database' => storage_path('testing/phpunit.sqlite'),
    'prefix'   => '',
 ]

and my phpunit.xml

    <php>
        <env name="APP_ENV" value="testing"/>
        <env name="DB_CONNECTION" value="testing"/>
        <env name="CACHE_DRIVER" value="array"/>
        <env name="SESSION_DRIVER" value="array"/>
        <env name="QUEUE_DRIVER" value="sync"/>
    </php>

With this we saved around 90% of time running our test cases.

lindyhopchris commented 7 years ago

Also confirm this issue on PostgresSQL 9.4

@jycr753 tip is good to use sqlite - we generally do that but in this instance we're trying to test specifically against Postgres as that is what is used in production so makes more sense to integration test with the "real" database.

ryrobbo commented 7 years ago

For anyone coming across this issue now, I've followed @Kyslik 's advice and just manually set a high connections value (set global max_connections = 800;).

Not the perfect solution but it's allowing me to run all of my tests.

coquer commented 7 years ago

@ryrobbo the problem with that is that it does become a exponential issue, before I made the jump to sqlite for testing I was setting the connections to 4000

bryandatanerds commented 7 years ago

We are using Doctrine via Connection::getDoctrineSchemaManager(), and it seems like the Doctrine connection (and hence the PDO) doesn't get cleaned up at the end of each of our unit tests, despite going \DB::purge(...). If we do the following instead, it solves this issue for us:

$connection->getDoctrineConnection()->close();
\DB::purge($connectionName ?: null);

Is this perhaps the issue you're facing as well?

And is it intentional to have to clean up a Doctrine connection like this?

(PHP 7.1, laravel/framework 5.4.27, doctrine/dbal 2.5.12, Postgres 9.5.7)

Kyslik commented 7 years ago

@jycr753 There is always possibility to use sqlite driver with :memory: database, which is perfect for testing.

'testing' => [
    'driver' => 'sqlite',
    'database' => ':memory:',
    'prefix' => '',
],

But remember to enable constrains checks https://sqlite.org/foreignkeys.html

tonychuuy commented 7 years ago

I'm getting this error in Laravel 5.5.2, I'm not using DatabaseTransactions trait, I have a dedicated database to run integration tests.

Laravel Version: 5.5.2 PHP Version: 7.1 Database Driver & Version: MySQL

Running a test suite of 234 tests.

tonychuuy commented 7 years ago

Adding this option in the testing database solve the problem in Laravel 5.5.2

'options'   => array(
    PDO::ATTR_PERSISTENT => env('DB_PERSISTENT', true),
),
Kyslik commented 6 years ago

@themsaid is this still an issue? #20340 was merged and people over there reported that it works.

I guess this can be closed.

IlCallo commented 6 years ago

Still getting this error in L5.4 from a test suite of 148 tests and going up.

Laravel Version: 5.4.36 PHP Version: 7.1 Database Driver & Version: MySQL PHPUnit 6.4.4

@avirdz solution worked, but it's obviously a workaround

I think this is related to the fact that I forced the application creation into TestCase constructor

public function __construct($name = null, array $data = [], $dataName = '')
{
    parent::__construct($name, $data, $dataName);

    $this->createApplication();
}

in order to use factories and access data already present in the database into dataProviders. Too many connections error is fired only for the test group which uses dataProviders and for all tests inside that group, so I guess it reaches the maximum connections right when generating the dataProviders output.

Dunno if I should open a new issue or keep with this one. Dunno also if this problem can be solved on the Laravel side: the cause is PHPUnit code design and workflow.

Kyslik commented 6 years ago

@IlCallo (great name btw, and I am being ironic...) L5.4 is not supported anymore.

IlCallo commented 6 years ago

Of course, but the problem could be present also on 5.5, I'll check when upgrading. And @avirdz said he experienced a "Too many connection" on 5.5.2

GuidoHendriks commented 6 years ago

I'm currently experiencing this problem on 5.5.24.

Adding this to my config solved it:

'options'   => array(
    PDO::ATTR_PERSISTENT => env('DB_PERSISTENT', true),
),
GuidoHendriks commented 6 years ago

Persistent DB did not solve it after all. Original solution from @ryrobbo did.

mfn commented 6 years ago

Many of things here sound similar to https://github.com/laravel/framework/issues/18056 . The major difference being: the other is talking about DatabaseTransactions, this one only mentions DatabaseMigrations.

From what I read here, this issue suffers the same problem in the end: connections are not free'd back.

Can anyone affected here try and use the DatabaseTransactions together with the migrations and see if it "fixes" it? Afaik MySQL doesn't support DDLs in transactions but \Illuminate\Foundation\Testing\TestCase::setUpTraits first runs the migrations and then activates the transactions (which should close the connections).

GuidoHendriks commented 6 years ago

I just noticed that both RefreshDatabase and DatabaseTransactions use similar methods called beginDatabaseTransaction. They do exactly the same, except for 1 thing: DatabaseTransactions not only rolls back, but disconnects as well. Where RefreshDatabase only rolls back.

https://github.com/laravel/framework/blob/5.5/src/Illuminate/Foundation/Testing/RefreshDatabase.php https://github.com/laravel/framework/blob/5.5/src/Illuminate/Foundation/Testing/DatabaseTransactions.php

I can’t test it at this moment. But I’m pretty this is the problem. The solution by the issue starter fixes it, and that just adds the missing disconnect.

nesk commented 6 years ago

@GuidoHendriks was right, I've just replaced:

$database->connection($name)->rollBack();

by:

$connection = $database->connection($name);

$connection->rollBack();
$connection->disconnect();

in the RefreshDatabase trait, and now it works like a charm! I wrote a PR (#22569) to fix that.