swoole-bundle
swoole-bundle copied to clipboard
Add DoctrinePingConnectionsHandler to check and reconnect connections before handling request
If workers stays idle for a long time period, connection to the DB may time out and get closed. It's a fairly common practice to test a connection and attempt a reconnect if connection is dead before processing a message from the queue in long lived workers. See examples: https://github.com/php-enqueue/enqueue-dev/blob/master/pkg/enqueue-bundle/Consumption/Extension/DoctrinePingConnectionExtension.php https://github.com/symfony/doctrine-bridge/blob/master/Messenger/DoctrinePingConnectionMiddleware.php In this PR I suggest using the same approach when handling requests with Swoole.
You may notice that solution in symfony/doctrine-bridge is slightly different. It was changed recently because Doctrine\DBAL\Connection::ping()
had been deprecated.
So technically this PR relies on the deprecated feature. But I believe by the time doctrine 3 is released with that feature removed we will see a much better solution in Symfony. The most reliable solution would probably be something like https://github.com/facile-it/doctrine-mysql-come-back - a connection wrapper, which reconnects on error. We may see some similar connection management solution in Symfony instead of current ping
method avoidance workaround by the time Doctrine 3 is released, so I wouldn't consider using this deprecated method a too bad solution for now (unless it's a huge lib/framework like Symfony where you want to keep things as future-proof as possible at all times).
But if you don't want deprecated features to be used, I could change this PR to use the same solution as in symfony/doctrine-bridge.
Please add some description of why is it need, and what problem does it solve. Also, you have a typo in first commit: " feat(request-handler): Add a handler to check and reconnect DB connections "
Does it solve the problems like https://github.com/k911/swoole-bundle/blob/develop/src/Bridge/Doctrine/ORM/EntityManagerHandler.php but in a more general way?
Codecov Report
Merging #322 (2cc9a70) into develop (cd61fad) will decrease coverage by
0.58%
. The diff coverage is63.93%
.
@@ Coverage Diff @@
## develop #322 +/- ##
=============================================
- Coverage 85.64% 85.06% -0.59%
- Complexity 644 659 +15
=============================================
Files 91 93 +2
Lines 2027 2082 +55
=============================================
+ Hits 1736 1771 +35
- Misses 291 311 +20
Impacted Files | Coverage Δ | Complexity Δ | |
---|---|---|---|
src/Bridge/Doctrine/ORM/EntityManagerHandler.php | 100.00% <ø> (ø) |
3.00 <0.00> (ø) |
|
...ony/Bundle/DependencyInjection/SwooleExtension.php | 71.04% <30.00%> (-6.12%) |
60.00 <3.00> (+6.00) |
:arrow_down: |
...ge/Doctrine/ORM/DoctrinePingConnectionsHandler.php | 92.85% <92.85%> (ø) |
6.00 <6.00> (?) |
|
.../Bridge/Doctrine/ORM/ClearEntityManagerHandler.php | 100.00% <100.00%> (ø) |
3.00 <3.00> (?) |
|
...mfony/Bundle/DependencyInjection/Configuration.php | 94.54% <100.00%> (+0.25%) |
10.00 <0.00> (ø) |
Hi @k911 Sorry for not adding description, I forgot to post it as a draft initially. I've added a description.
feat(request-handler): Add a handler to check and reconnect DB connections
Commit message is fixed.
Does it solve the problems like https://github.com/k911/swoole-bundle/blob/develop/src/Bridge/Doctrine/ORM/EntityManagerHandler.php but in a more general way?
Nope. It checks that DB connection is alive before processing request, while EntityManagerHandler resets the internal state of the EntityManager after request is handled. BTW I have some improvements for the EntityManagerHandler too, I'll send a PR within a few days.
It checks that DB connection is alive before processing request, while EntityManagerHandler resets the internal state of the EntityManager after request is handled.
@supersmile2009 I was referring to this part: https://github.com/k911/swoole-bundle/blob/develop/src/Bridge/Doctrine/ORM/EntityManagerHandler.php#L30-L33
But if you don't want deprecated features to be used, I could change this PR to use the same solution as in symfony/doctrine-bridge.
And yes, please if there is a way to avoid adding deprecated features it would be great.
Hi @k911.
I finally found some time to update this PR.
So yeah, the new handlers take care of the connection pinging and clearing entity manager (similar to what K911\Swoole\Bridge\Doctrine\ORM\EntityManagerHandler
does), but in a more general/versatile way, since it is possible to have multiple EntityManagers and/or Connections configured.
Also pinging logic now avoids using deprecated ping
method.
Originally I wanted to submit pinging and manager clearing changes as 2 separate PRs, but they seem to be fairly related, so I ended up pushing all of the changes here. These 2 new handlers combined are a feature-complete replacement for EntityManagerHandler.
Old EntityManagerHandler isn't used and is marked as deprecated. Since it's not internal, it can't be removed without bumping a major version. When entity_manager_handler
option is configured, under the hood it will set doctrine_ping_connections_handler
and clear_entity_manager_handler
to activate/deactivate new handlers to maintain compatibility.
Code Climate has analyzed commit 2cc9a708 and detected 0 issues on this pull request.
The test coverage on the diff in this pull request is 66.6% (50% is the threshold).
This pull request will bring the total coverage in the repository to 85.1% (-0.5% change).
View more on Code Climate.
Hi @supersmile2009, thanks for PR and your hard work.
I've run CI here and unfortunately I see two failures at least:
- we have to bump doctrine because
lowest
build fails, so most likely in dev dependencies we'll have to require version that works with your code - run
composer fix