swoole-bundle icon indicating copy to clipboard operation
swoole-bundle copied to clipboard

Add DoctrinePingConnectionsHandler to check and reconnect connections before handling request

Open supersmile2009 opened this issue 4 years ago • 7 comments

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.

supersmile2009 avatar Sep 01 '20 12:09 supersmile2009

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?

k911 avatar Sep 01 '20 12:09 k911

Codecov Report

Merging #322 (2cc9a70) into develop (cd61fad) will decrease coverage by 0.58%. The diff coverage is 63.93%.

Impacted file tree graph

@@              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> (ø)

codecov[bot] avatar Sep 01 '20 13:09 codecov[bot]

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.

supersmile2009 avatar Sep 02 '20 13:09 supersmile2009

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.

k911 avatar Sep 02 '20 18:09 k911

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.

supersmile2009 avatar Jan 18 '21 20:01 supersmile2009

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.

codeclimate[bot] avatar Jan 26 '21 22:01 codeclimate[bot]

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

k911 avatar Jan 28 '21 19:01 k911