framework

framework copied to clipboard

Background

PDO connection doesn't automatically get closed when the application is unset but instead requires explicitly set to null to trigger connection close:

https://www.php.net/manual/en/pdo.connections.php

Since PHPUnit is especially a long-running process when the application gets booted and terminated on each test the PDO may persist and eventually can cause max database connections. To solve we have two options:

Option A: We can explicitly disconnect the database between tests

Disadvantages:

• Caused breaking change to the existing application since it is still possible to use PDO and loaded instances (without IoC) after Iluminate\Foundation\Testing\TestCase::tearDown() has been called.
• Any cached/static instances holding PDO will still prevent the database from being disconnected.

Option B: Reuse PDO instances between tests (provided in this PR)

Disadvantage:

• The only issue I can think of if tests within a TestCase change database configuration between tests, since the PDO instance is loaded using cache it not going to fetch the specific configuration defined for the test. However, I don't see any reason why a developer would have this for the application tests, even for package development this would be a rare requirement.

To cover Option A, you can configure a base TestCase and set $disconnectDatabaseConnections property to true:

<?php

namespace Tests;

abstract class TestCase extends \Illuminate\Foundation\Testing\TestCase
{
    use CreatesApplication;

+   protected $disconnectDatabaseConnections = true;

}

Related to #49373? Please comment there

Since this applies to code that falls outside the scope of Suggested change , I would like to comment on it separately. (I'm not familiar with MySQL, so I'd like to comment only on PostgreSQL. But I think MySQL probably has similar features as well.)

https://github.com/laravel/framework/pull/49385#discussion_r1427684362

There is a risk that the transaction state and session settings will be carried over

There is some changeable state that persists while a session is open, whether inside or outside a transaction, and whether commit or rollback.

However, certain status also allow us to explicitly discard their state. (Some implementations may already exist. Sorry if there are.)

• Session Authorization
  • Return to initial state with RESET SESSION AUTHORIZATION.
• Connection status settings.
  • RESET ALL resets everything. (Also valid for many settings other than SET statements)
  • PHP's ext/pgsql extension uses this method to reset the state of persistent connections. I think it's a good idea to implement the same in our framework.
• Advisory lock, Named Prepared Statement
  • All will be discarded with DISCARD ALL .

In order to preserve existing behavior, we should execute these (or other?) statements to return the session state to its initial state as much as possible.

---

UPDATE: 2023-12-16 9:52 JST

This problem can now be resolved by an option implemented after the first mention. Thank you!

To cover Option A, you can configure a base TestCase and set $disconnectDatabaseConnections property to true:

Since PHPUnit is especially a long-running process when the application gets booted and terminated on each test the PDO may persist and eventually can cause max database connections.

What causes it to persist? How do you recreate this on a fresh Laravel application to hit the max connections error?

What causes it to persist?

Mentioned in https://github.com/laravel/framework/discussions/49373#discussioncomment-7854320. It appears that a reference to the connection resource is being held in some code somewhere, but I don't know where. It may be Laravel code, but it may also be processed by PHPUnit.

How do you recreate this on a fresh Laravel application to hit the max connections error?

Mentioned in #49311. It happens by simply repeating a test case.

What causes it to persist? How do you recreate this on a fresh Laravel application to hit the max connections error?

Before

We assume using flush() and unset($app) would be enough to clear PDO reference but it doesn't seem so (which led me to believe that the documentation in PHP meant that we do need to explicitly set PDO instances to null.

After

We tried disconnecting the database connection in tearDown via #49327 but this caused a regression (reported via #49359) and proven that you can still interacts with PDO after unset($app)

Sorry, but if the current behavior is a significant problem, why are we not seeing many more reports of this from basically every Laravel application in the world that has tests?

@taylorotwell Possibly because everyone uses RefreshDatabase. But in some reason I prefer DatabaseTruncation, so I critically encountered the problem.

@taylorotwell

Here are simple steps to reproduce this problem.

The following test code:

<?php

namespace Tests\Feature;

use Tests\TestCase;
use Illuminate\Foundation\Testing\DatabaseMigrations;
use Illuminate\Foundation\Testing\DatabaseTruncation;

class ConnTest extends TestCase
{
    use DatabaseMigrations;
    // or use DatabaseTruncation;
    public static function repeat(): array
    {
        return array_fill(0, 10, []);
    }

    /** @dataProvider repeat */
    public function test(): void
    {
        echo "waiting input...";
        ob_flush();
        fgets(STDIN);
        echo "input received\n";
        $this->assertTrue(true);
    }
}

run test:

php artisan test tests/Feature/ConnTest.php

The test will stop until the Enter key is pressed:

waiting input...
input received
waiting input...
input received
waiting input...

...(omit)

For MySQL, you can check the number of connections with the following query:

show status like 'Threads_connected';

If you check the number of connections while stopping the test, you will see that the number of connections increases as the test case progresses:

mysql> show status like 'Threads_connected';
+-------------------+-------+
| Variable_name     | Value |
+-------------------+-------+
| Threads_connected | 2     |
+-------------------+-------+
1 row in set (0.00 sec)

mysql> show status like 'Threads_connected';
+-------------------+-------+
| Variable_name     | Value |
+-------------------+-------+
| Threads_connected | 3     |
+-------------------+-------+
1 row in set (0.00 sec)

...(omit)

I found the reason for @taylorotwell's question.

In short, with the current implementation, whether the problem occurs or not depends on when PHP's circular reference collector is triggered.

It varies depending on the PHP settings and the memory capacity of the execution environment, so

• The code shown by @rikvdh may not always work.
• The problem that @mpyw actually encountered may not be reproduced depending on the environment.

In any case, the current implementation's behavior changes depending on the environment (or luck), and this pull request will resolve that.

I have prepared CI results in my repository that can confirm the above operation. Complete results can be found here.

This image visualizes the increase and decrease in the number of simultaneous connections by repeating the same test. $app->flush() is called every time, but the actual timing of disconnection is different each time.

Complete CI results also include cases where we explicitly call gc_collect_cycles(). Since the number of connections in this case is constant, we can see that the trigger for disconnection is GC.

I'd like to chime in on this since it seems like it may heavily affect me.

The only issue I can think of if tests within a TestCase change database configuration between tests, since the PDO instance is loaded using cache it not going to fetch the specific configuration defined for the test. However, I don't see any reason why a developer would have this for the application tests, even for package development this would be a rare requirement.

The reason I have is because I run a multi-database multi-tenant application and sometimes I have some critical features that need the extra level of security to ensure that:

1. Given 2 tenants (2 databases) and 1 user in each tenant
2. When User A performs an action
3. It reflects on Tenant's database without crossing/impacting the tenant boundary.

---

From reading the PR, I see Taylor's point. I've been using Laravel since 5.0 and I have never had any max connection issues. I believe many Laravel users don't face this issue at all. Making a significantly drastic change to how the framework handles database connections affecting every Laravel/Phpunit Users to fix an edge case without a way to keep the existing behavior seems like an opportunity to invite many more issues/bug reports than the current situation.

I'm all for reusing the same PDO connection across many tests which could improve performance and speed up tests, but I feel like users need to be able to better control this.

While we're on this subject, a paper cut issue I've had with Laravel for many years is precisely how DatabaseManager is tied with Config Repository, making it unable to work without global state changes that causes confusion.

If something like my personal desire could be implemented, then the disadvantage listed here would be way less relevant. From my understanding, the disadvantage listed is caused exactly by my "paper cut" mention that you need to manipulate a global state on Config Repository prior to establishing a connection with the DatabaseManager, otherwise any changes to the global Config Repository will not reflect on the DatabaseManager connections already established.

If we expose a establishUsing(string $name, array $config, bool $throwsIfAlreadyConnected = true) on the DatabaseManager in a way that we no longer rely on Config Repository, then it completely removes the surprise-factor when a Laravel user wants to hot swap connection configuration on the DatabaseManager without needing to interact with the global Repository Config.

Note: the method signature here is just a prototype. I don't really expect to have a $throwsIfAlreadyConnected parameter, my goal was more to express the desire allow Laravel users to decide whether to override existing connections on-the-fly or throw an exception if they try to override an existing connection without knowing.

This would also make it easier to use the Migrator class outside of Artisan and we could work out the ability to $migrator->runOn([$migrationsPath], $connectionName, $connectionConfiguration) without having to DB::purge() and then Config::set() (this is something we could easily accomplish on user land code without Laravel needing to get involved).

@taylorotwell Possibly because everyone uses RefreshDatabase. But in some reason I prefer DatabaseTruncation, so I critically encountered the problem.

@mpyw If this problem mainly affects DatabaseTruncation, can't it be fixed inside DatabaseTruncation trait?

I believe many Laravel users don't face this issue at all.

It all depends on the max connection configured locally and on CI, we did face this issue on Framework own tests a few years back: https://github.com/laravel/framework/pull/39877

I'm all for reusing the same PDO connection across many tests which could improve performance and speed up tests, but I feel like users need to be able to better control this.

Welcome any suggestion on this, we did add ability to disconnect the database connection via a property flag in this PR.

If this problem mainly affects DatabaseTruncation, can't it be fixed inside DatabaseTruncation trait?

If the approach is to disconnect all database during tearDown, this have been proposed and rejected, also potentially impacts performance (since we only disconnect database connectio via RefreshDatabase and DatabaseMigrations trait at the moment) with less control.

Would it be simpler and less breaking (particularly for the multi-tenant scenario) to just call gc_collect_cycles periodically while running tests?

If the approach is to disconnect all database during tearDown, this have been proposed and rejected, also potentially impacts performance (since we only disconnect database connectio via RefreshDatabase and DatabaseMigrations trait at the moment) with less control.

I guess my primary point is: I'm a user of the DatabaseTransactions trait and as such it seems I'm more unlikely to face this problem than users of the DatabaseTruncation as it was mentioned.

Perhaps the implemented solution to the problem can be more targeted at where the problem is and leave users that are outside the scope of it unaffected. The problem with the current proposal is that it completely hijacks the DatabaseConnectionFactory at a global level (OverrideProvidersForTesting), causing 100% of Laravel installation to be affected by this change regardless of whether they are subject to the reported bug or not.

@crynobone

Would it be simpler and less breaking (particularly for the multi-tenant scenario) to just call gc_collect_cycles periodically while running tests?

Is this the answer?