Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

[8.x] Fix Doctrine type mappings creating too many connections #40303

Merged
merged 7 commits into from
Jan 8, 2022
Merged

[8.x] Fix Doctrine type mappings creating too many connections #40303

merged 7 commits into from
Jan 8, 2022

Conversation

bakerkretzmar
Copy link
Contributor

TLDR: this PR closes #40297 by only registering custom Doctrine type mappings on a Doctrine connection after that connection is created (as opposed to creating one to register them).

Context

My and @TomHAnderson's original PR, #40119, created an issue with tests where way too many database connections would be created. We were manually creating a database connection to register the custom type mappings, which caused database connections to be created every time the app boots, and dozens of unnecessary connections to be created when tests are run in parallel.

I'm still not entirely sure why all those connections stick around, I tried manually closing/destroying them through Doctrine and that didn't seem to work.

@bakerkretzmar
Copy link
Contributor Author

@canvural @skollro if either of you have feedback about this or are able to test it that would be greatly appreciated 🙏🏻

@canvural
Copy link
Contributor

canvural commented Jan 7, 2022

@bakerkretzmar I confirm this fixes it! Thank you! Couldn't test with parallel though.

@skollro
Copy link
Contributor

skollro commented Jan 7, 2022

@bakerkretzmar I've just tried your proposed solution.
Registering a custom type via database.dbal.types works in sequential and parallel tests now.
DB::registerDoctrineType() in the AppServiceProvider's register method works for sequential tests, but not for parallel tests.

@bakerkretzmar
Copy link
Contributor Author

DB::registerDoctrineType() in the AppServiceProvider's register method works for sequential tests, but not for parallel tests.

@skollro does it work if you do it in the boot method?

@skollro
Copy link
Contributor

skollro commented Jan 7, 2022

@bakerkretzmar unfortunately, no. It seems like getDoctrineConnection isn't even called in parallel test environments 🤔

@bakerkretzmar
Copy link
Contributor Author

@skollro thanks. It does get called but not where I thought it did, working on fixing this now.

$this->getDoctrineSchemaManager()
->getDatabasePlatform()
->registerDoctrineTypeMapping($type, $name);
$this->doctrineTypeMappings[$name] = $type;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've kept this whole registerDoctrineType method for backwards compatibility but it can be removed in Laravel 9 because the same method on the DatabaseManager class replaces it. I'll PR that today too.

@bakerkretzmar bakerkretzmar marked this pull request as ready for review January 7, 2022 21:31
@bakerkretzmar
Copy link
Contributor Author

bakerkretzmar commented Jan 7, 2022

I'm pretty sure this fixes the issue now in all the cases mentioned here and in various comments: registering custom types either via database.dbal.types or with DB::registerDoctrineType() in a service provider, and testing either normally or in parallel.

There is some code duplication here to maintain complete backwards compatibility, I'll submit another PR asap to clean this up a bit for Laravel 9. Basically, the type registration has to happen through the DatabaseManager so that it persists across test runs properly in parallel testing, but every single new connection that gets created also has to be provided with the types. If there's a better way to do that than to store them in an array, I'm all ears 😅

@driesvints
Copy link
Member

Thanks @bakerkretzmar & @TomHAnderson !

@skollro
Copy link
Contributor

skollro commented Jan 8, 2022

Thanks! Can confirm DB::registerDoctrineType() and database.dbal.types also works with parallel testing now.
But the backwards-compatible DB::connection()->registerDoctrineType() still breaks parallel tests.

@bakerkretzmar
Copy link
Contributor Author

bakerkretzmar commented Jan 8, 2022

Yeah I'm going to remove DB::connection()->registerDoctrineType() if possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#40119 broke how dbal array in config/database.php is handled.
5 participants