[Tests] How do we fix connection leaks when using DatabaseTruncation or DatabaseMigrations?

[mpyw]

Overview

In recent discussions and PRs, we've encountered challenges with handling database connections in tests, particularly when using DatabaseTruncation or DatabaseMigrations. The core issue revolves around ensuring that database connections are properly closed after tests, without causing unintended side effects in the test teardown process. This poll seeks community input on the best approach to address this issue.

• Some traits of database tests do not close the connection even after the test is finished #49311
• [10.x] Disconnecting the database connection after testing #49327
• Interacting with the database in the test teardown after calling the parent teardown is broken #49359
• [10.x] Revert "[10.x] Disconnecting the database connection after testing" #49361
• [11.x] Disconnecting the database connection after testing  #49362

Options

Implement Disconnection in tearDown()

Re-attempt the fix by disconnecting the database connection in the tearDown() method and propose it again for the 11.x branch. It's important to adhere to the proper call order of parent::tearDown(), as not doing so led to the issue raised in:

• Interacting with the database in the test teardown after calling the parent teardown is broken #49359
  • Interacting with the database in the test teardown after calling the parent teardown is broken #49359 (comment)
  • Interacting with the database in the test teardown after calling the parent teardown is broken #49359 (comment)

@rikvdh I think this is expected behavior:

• parent::setUp() must be called first

  public function setUp(): void
  {
      parent::setUp();
      // Your Code Goes Here
  }

• parent::tearDown() must be called later

  public function tearDown(): void
  {
      // Your Code Goes Here
      parent::tearDown();
  }

While this was a significant change, your original code has a major problem.

@driesvints @taylorotwell Why did you closed #49362?

It is odd that model deletion works after parent::tearDown(), since facades are deactivated after parent::tearDown().

• This doesn't work:

  public function tearDown(): void
  {
      parent::tearDown();
      DB::table($this->model->getTable())->where('id', $this->model->id)->delete();
  }

• UNEXPECEDLY this works:

  public function tearDown(): void
  {
      parent::tearDown();
      $this->model->delete();
  }

Use beforeApplicationDestroyed() in setUp()

Propose the fix by utilizing beforeApplicationDestroyed() within the setUp() method and re-submit for the 11.x branch. This might have broader implications due to compatibility issues with orchestra/testbench. As explained by @KentarouTakeda in:

• [10.x] Disconnecting the database connection after testing #49327
  • [10.x] Disconnecting the database connection after testing #49327 (comment)

I found that tests using orchestral/testbench do not work properly if disconnected during DatabaseMigrations or DatabaseTruncation.

Update Documentation and Delegate to Users

Amend comments in DatabaseTruncation and DatabaseMigrations and update the official documentation to guide users to handle this issue on their end. This approach avoids changes to the framework itself and places the responsibility on the users.

Other

Please specify any alternative suggestions or insights.

[rikvdh]

I've never used DatabaseTruncation nor DatabaseMigrations, our DB is just too big for this (migrations take 7-8 min). But never experienced problems with connection leaks. I'm on MariaDB/MySQL, might this be PG specific?

[mpyw]

• You are using RefreshDatabase or DatabaseTransactions, right? If so, you are correct in assuming that the problem is not reproducible since an explicit disconnect has already been included. This is independent of the RDBMS type.
• Or are you not using any traits, but have less than 151 test cases?
  • MySQL max_connections default: 151
  • Postgres max_connections default: 100

Recently our company has found that the automatic transactions that RefreshDatabase puts up are not compatible with native database functions (e.g. Postgres' pg_advisory_xact_lock), so we have changed to a more realistic DatabaseTruncation. As a result, we have just encountered this connection leak problem.

[rikvdh]

we use DatabaseTransactions. I'm not working with other traits atm.

[mpyw]

DatabaseTransactions has disconnection logic here:

framework/src/Illuminate/Foundation/Testing/DatabaseTransactions.php

Line 36 in eb94883

$connection->disconnect();

[SakiTakamachi]

Hi, I have discovered an issue that may be related to context discussed.

1. Added destructor to Illuminate\Foundation\Application.

public function __destruct()
{
    var_dump('die');
}

2. Run a test like this:

class BugTest extends TestCase
{
    public static function repeat(): array
    {
         return array_fill(0, 4, []);
    }

    /** @dataProvider repeat */
    public function test(): void
    {
        echo "-----------\n";
        $this->assertTrue(true);
    }
}

I expected the following result:

-----------
string(3) "die"
-----------
string(3) "die"
-----------
string(3) "die"
-----------
string(3) "die"

   PASS  Tests\Feature\BugTest

  Tests:    1 passed (4 assertions)
  Duration: 0.11s

However, I actually got the following result:

-----------
-----------
-----------
-----------

   PASS  Tests\Feature\BugTest

  Tests:    1 passed (4 assertions)
  Duration: 0.11s

string(3) "die"
string(3) "die"
string(3) "die"
string(3) "die"
string(3) "die"

From this result, it appears that the Application instance increment each time the test method is executed, and is never destroyed until the end.

---

edit)

It looks like it has been cleaned up because flush() is called during tear down, but I still think it's strange that gc doesn't release it.

Therefore, rather than saying that the connection remains because the Application instance remains, my opinion is that there seems to be a hidden place somewhere where the instance reference is still retained.

[SakiTakamachi]

I took a closer look. It seems to be quite difficult to identify the cause of the GC release leak.

Rather than spending time there, use tearDown() to temporarily put the DB connection instance into a property, and then completely disconnect the DB connection just before createApplication() in refreshApplication(), which is executed in the next setUp(). We might be able to solve this problem by doing this.

To summarize briefly, the change is basically the same as @KentarouTakeda but it move the timing to purge the DB connection to the next setUp() instead of tearDown().

---

It may not be able to keep it even if you put it in a property. I wish there was a better way...

[mpyw]

Thank you for your detailed investigation. I would like to focus only on the more serious issue of the number of database connections, as it would have a significant impact on trying to completely solve the leakage of garbage collection.

[mpyw]

Isn't it re-submitting #49362 enough?

[SakiTakamachi]

Thank you for your detailed investigation. I would like to focus only on the more serious issue of the number of database connections, as it would have a significant impact on trying to completely solve the leakage of garbage collection.

Agree. Even if we can fix the leak in your application, it will probably cause the same behavior as the "BC break" mentioned in #49359.

Isn't it re-submitting #49362 enough?

First of all, I would like to express my opinion. I believe that the contents of #49362 are appropriate for changes to 11.x. There is no reason for a framework to tolerate incorrect usage of tearDown().

However, in that case, the connection leak will continue to exist in 10.x. 10.x is supported until August 2024, so support for users is still required for a while.

Personally, I think it might be a good idea to consider an interim target that doesn't cause a "BC Break" to 10.x.

[mpyw]

👀

• [11.x] Allows to persist database connection between tests and only reset PDO in afterClassTearDown() #49385

[KentarouTakeda]

The newly proposed $disconnectDatabaseConnections option provides the same behavior as Implement Disconnection in tearDown().

The connection leak has been fixed even if you do not specify that option. Many of the issues will be resolved once this pull request is merged.