[DoctrineBridge] Idle connection listener for long running runtime #53214

alli83 · 2023-12-26T11:42:34Z

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #51661
License	MIT

This pull request introduces a solution based on the RoadRunner bundle's Doctrine ORM/ODM middleware https://github.com/Baldinof/roadrunner-bundle/blob/3.x/src/Integration/Doctrine/DoctrineORMMiddleware.php#L22.
It checks the status of Doctrine connection, then if the connection is initialized and connected, it performs a 'ping' to check its viability. If the ping fails, it closes the connection.

linked to doctrine/DoctrineBundle#1739

alexandre-daubois

A few suggestions 🙂

src/Symfony/Bridge/Doctrine/Listener/DoctrineConnectionListener.php

nicolas-grekas · 2023-12-26T13:43:03Z

Hi, thanks for working on this topic.
Would there be a way to put that concern in Doctrine itself, by decorating the Connection object?
I'm concerned that the current approach will trigger one DB call per HTTP request while not all HTTP entpoints might use Doctrine, and also by the fact this only works for HTTP while recovering from stalled DB connections is also a concern for CLI workers.

OskarStark · 2023-12-26T21:19:08Z

Just reading the code, it adds a lot new stuff which looks like a new feature instead of a bugfix to me

dunglas · 2023-12-27T06:20:21Z

I agree that a decorator would be better. Also, would it be possible to catch errors and reconnect only in this case instead of triggering a ping? This would remove the performance hit in most cases.

@OskarStark the code probably needs to be simplified, but the underlying bug is annoying because it makes Symfony apps unstable when using FrankenPHP, RoadRunner (without the bundle), Swoole etc. The fix should be made at least in the current stable version.

alli83 · 2023-12-27T06:40:28Z

@nicolas-grekas @dunglas ok I'll update it and try to decorate the Connection object

OskarStark · 2023-12-27T08:50:30Z

@OskarStark the code probably needs to be simplified, but the underlying bug is annoying because it makes Symfony apps unstable when using FrankenPHP, RoadRunner (without the bundle), Swoole etc. The fix should be made at least in the current stable version.

Thanks for the explanation Kevin 👍

ostrolucky

I don't like the approach, because as @nicolas-grekas mentioned, this will keep pinging the DB every request, even if normally request wouldn't need connection. Can't we make this DBAL middleware instead? Middleware would check if any query fails with "DB went away..." error, if so, reset everything and retry.

Also, bear in mind counterpart for messenger is at https://github.com/symfony/symfony/blob/412ea7afcb533b8a7a95e248d741b70fea505350/src/Symfony/Bridge/Doctrine/Messenger/DoctrinePingConnectionMiddleware.php, so this would be a good opportunity to extract common parts into bridge

src/Symfony/Bridge/Doctrine/Listener/DoctrineConnectionSubscriber.php

alli83 · 2024-01-02T03:04:41Z

I don't like the approach, because as @nicolas-grekas mentioned, this will keep pinging the DB every request, even if normally request wouldn't need connection. Can't we make this DBAL middleware instead? Middleware would check if any query fails with "DB went away..." error, if so, reset everything and retry.

Also, bear in mind counterpart for messenger is at https://github.com/symfony/symfony/blob/412ea7afcb533b8a7a95e248d741b70fea505350/src/Symfony/Bridge/Doctrine/Messenger/DoctrinePingConnectionMiddleware.php, so this would be a good opportunity to extract common parts into bridge

Ok, I'll update then

alli83 · 2024-01-03T06:20:17Z

@ostrolucky
Before refactoring (taking into account DoctrinePingConnectionMiddleware), I'm not entirely sure I grasp the specific middleware you're referring to. Are you referring to those related to Doctrine\DBAL\Driver
Doctrine\DBAL\Driver\Connection

ostrolucky · 2024-01-03T08:20:26Z

I was referring to middlewares that implement Doctrine\DBAL\Driver\Middleware. This is how Symfony does stuff like logging

KDederichs · 2024-02-07T20:39:44Z

Any news on this/what's preventing it from being merged?

src/Symfony/Bridge/Doctrine/Middleware/IdleConnection/Listener.php

nicolas-grekas

I force-pushed to fix my last comments :)

dunglas

🎉

nicolas-grekas · 2024-04-25T09:30:48Z

Thank you @alli83.

andersonamuller · 2024-04-25T16:34:57Z

src/Symfony/Bridge/Doctrine/phpunit.xml.dist

+                    <element key="time-sensitive">
+                        <array>
+                            <element key="0"><string>Symfony\Bridge\Doctrine\Middleware\Debug</string></element>
+                            <element key="1"><string>Symfony\Bridge\Doctrine\Middleware\Debug</string></element>


@nicolas-grekas Isn't Symfony\Bridge\Doctrine\Middleware\IdleConnection?

Oops, PR welcome

fixed in 65ccca0

calinblaga · 2024-06-05T12:55:33Z

src/Symfony/Bridge/Doctrine/Tests/Middleware/IdleConnection/ListenerTest.php

+        $containerMock = $this->createMock(ContainerInterface::class);
+        $connectionExpiries = new \ArrayObject(['connectionone' => time() - 30, 'connectiontwo' => time() + 40]);
+
+        $connectionOneMock = $this->getMockBuilder(ConnectionInterface::class)


You can just do $this->createMock(ConnectionInterface::class) to spare two lines of code.

Please don't comment on old, already merged PRs. If you feel strongly about this, send a PR instead.

Sure, was just a general suggestion, though.

jsamouh · 2025-01-16T19:18:26Z

possible to backport that into 6.X ? cc @fabpot

derrabus · 2025-01-16T19:19:34Z

No, we don't backport features.

jsamouh · 2025-01-16T21:31:00Z

well, does not seem to be a feature to me as it fix the error connection for long running runtime whis is our case right now in production with frankenPHP worker mode, cc @dunglas

greg0ire · 2025-01-16T21:46:34Z

We understand you are having an issue you would love to solve, but this listener is a new Symfony feature improving your experience with other software (Doctrine + frankenPHP). There was no issue with Symfony, so it's not a bugfix, and it really shows in the diff, which consists in new classes. It also shows in the changelog: "Add support for auto-closing idle connections"

Please stop pinging busy people that have bigger fish to fry and start work on upgrading to Symfony 7.1. If that's too much work, you can always copy paste the classes in this PR in your project, and wire them up.

jsamouh · 2025-01-16T23:04:50Z

I dont agree but if you say so
thks for the response !
Good luck

carsonbot added Status: Needs Review Bug DoctrineBridge labels Dec 26, 2023

carsonbot added this to the 6.4 milestone Dec 26, 2023

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch 3 times, most recently from 07aa62f to ba1eefd Compare December 26, 2023 11:53

alli83 mentioned this pull request Dec 26, 2023

Register the idle connection listener doctrine/DoctrineBundle#1739

Merged

alexandre-daubois reviewed Dec 26, 2023

View reviewed changes

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from ba1eefd to 3ec9cd7 Compare December 27, 2023 05:15

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from 3ec9cd7 to 24fb643 Compare December 27, 2023 06:38

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from 24fb643 to 40d646a Compare December 27, 2023 06:44

ostrolucky suggested changes Dec 30, 2023

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 30, 2023

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from 40d646a to eb35f9d Compare January 3, 2024 06:17

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch 2 times, most recently from 27e36c1 to d8773ac Compare January 3, 2024 06:23

dunglas mentioned this pull request Feb 7, 2024

Worker mode seems to lose doctrine connection after a while dunglas/frankenphp#561

Closed

dunglas mentioned this pull request Feb 25, 2024

FrankenPHP + Postgres: no connection to the server dunglas/frankenphp#438

Closed

alli83 force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from d8773ac to 7791eb2 Compare March 12, 2024 04:19

nicolas-grekas reviewed Apr 25, 2024

View reviewed changes

src/Symfony/Bridge/Doctrine/Middleware/IdleConnection/Listener.php Outdated Show resolved Hide resolved

nicolas-grekas modified the milestones: 6.4, 7.1 Apr 25, 2024

nicolas-grekas force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from b16e918 to 3d2816c Compare April 25, 2024 09:20

nicolas-grekas approved these changes Apr 25, 2024

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Apr 25, 2024

nicolas-grekas added Ready ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" labels Apr 25, 2024

dunglas approved these changes Apr 25, 2024

View reviewed changes

[DoctrineBridge] Idle connection listener for long running runtime

f7cc44e

nicolas-grekas force-pushed the doctrine-brige-doctrine-connection-listener-long-running-runtime branch from 3d2816c to f7cc44e Compare April 25, 2024 09:30

nicolas-grekas merged commit 462ab95 into symfony:7.1 Apr 25, 2024
8 of 10 checks passed

nicolas-grekas mentioned this pull request Apr 25, 2024

[DoctrineBridge] Idle connection listener for long running runtime symfony/symfony-docs#19827

Open

andersonamuller reviewed Apr 25, 2024

View reviewed changes

fabpot mentioned this pull request May 2, 2024

Release v7.1.0-BETA1 #54820

Merged

calinblaga reviewed Jun 5, 2024

View reviewed changes

n1k00las mentioned this pull request Jun 10, 2024

Mariadb Connexion lost dunglas/symfony-docker#638

Closed

ostrolucky mentioned this pull request Aug 18, 2024

"No Active Transaction" Error After Reconnecting Database in Worker Mode doctrine/DoctrineBundle#1815

Closed

jsamouh mentioned this pull request Feb 24, 2025

Persistent Connection and RDS proxy dunglas/frankenphp#1408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge] Idle connection listener for long running runtime #53214

[DoctrineBridge] Idle connection listener for long running runtime #53214

alli83 commented Dec 26, 2023 •

edited by nicolas-grekas

Loading

alexandre-daubois left a comment

nicolas-grekas commented Dec 26, 2023 •

edited

Loading

OskarStark commented Dec 26, 2023

dunglas commented Dec 27, 2023 •

edited

Loading

alli83 commented Dec 27, 2023

OskarStark commented Dec 27, 2023

ostrolucky left a comment •

edited

Loading

alli83 commented Jan 2, 2024

alli83 commented Jan 3, 2024 •

edited

Loading

ostrolucky commented Jan 3, 2024

KDederichs commented Feb 7, 2024

nicolas-grekas left a comment

dunglas left a comment

nicolas-grekas commented Apr 25, 2024

andersonamuller Apr 25, 2024

nicolas-grekas Apr 25, 2024

xabbuh Apr 26, 2024

calinblaga Jun 5, 2024

greg0ire Jun 5, 2024

calinblaga Jun 5, 2024

jsamouh commented Jan 16, 2025 •

edited

Loading

derrabus commented Jan 16, 2025

jsamouh commented Jan 16, 2025 •

edited

Loading

greg0ire commented Jan 16, 2025

jsamouh commented Jan 16, 2025 •

edited

Loading

[DoctrineBridge] Idle connection listener for long running runtime #53214

[DoctrineBridge] Idle connection listener for long running runtime #53214

Conversation

alli83 commented Dec 26, 2023 • edited by nicolas-grekas Loading

alexandre-daubois left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Dec 26, 2023 • edited Loading

OskarStark commented Dec 26, 2023

dunglas commented Dec 27, 2023 • edited Loading

alli83 commented Dec 27, 2023

OskarStark commented Dec 27, 2023

ostrolucky left a comment • edited Loading

Choose a reason for hiding this comment

alli83 commented Jan 2, 2024

alli83 commented Jan 3, 2024 • edited Loading

ostrolucky commented Jan 3, 2024

KDederichs commented Feb 7, 2024

nicolas-grekas left a comment

Choose a reason for hiding this comment

dunglas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 25, 2024

andersonamuller Apr 25, 2024

Choose a reason for hiding this comment

nicolas-grekas Apr 25, 2024

Choose a reason for hiding this comment

xabbuh Apr 26, 2024

Choose a reason for hiding this comment

calinblaga Jun 5, 2024

Choose a reason for hiding this comment

greg0ire Jun 5, 2024

Choose a reason for hiding this comment

calinblaga Jun 5, 2024

Choose a reason for hiding this comment

jsamouh commented Jan 16, 2025 • edited Loading

derrabus commented Jan 16, 2025

jsamouh commented Jan 16, 2025 • edited Loading

greg0ire commented Jan 16, 2025

jsamouh commented Jan 16, 2025 • edited Loading

alli83 commented Dec 26, 2023 •

edited by nicolas-grekas

Loading

nicolas-grekas commented Dec 26, 2023 •

edited

Loading

dunglas commented Dec 27, 2023 •

edited

Loading

ostrolucky left a comment •

edited

Loading

alli83 commented Jan 3, 2024 •

edited

Loading

jsamouh commented Jan 16, 2025 •

edited

Loading

jsamouh commented Jan 16, 2025 •

edited

Loading

jsamouh commented Jan 16, 2025 •

edited

Loading