Skip to content
This repository was archived by the owner on Jan 17, 2022. It is now read-only.

Add DoctrinePingConnectionsHandler to check and reconnect connections before handling request#322

Open
supersmile2009 wants to merge 5 commits into
k911:developfrom
supersmile2009:doctrine-ping-handler
Open

Add DoctrinePingConnectionsHandler to check and reconnect connections before handling request#322
supersmile2009 wants to merge 5 commits into
k911:developfrom
supersmile2009:doctrine-ping-handler

Conversation

@supersmile2009

@supersmile2009 supersmile2009 commented Sep 1, 2020

Copy link
Copy Markdown
Contributor

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.

@k911

k911 commented Sep 1, 2020

Copy link
Copy Markdown
Owner

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

codecov Bot commented Sep 1, 2020

Copy link
Copy Markdown

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) ⬇️
...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> (ø)

@supersmile2009

Copy link
Copy Markdown
Contributor Author

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.

@k911

k911 commented Sep 2, 2020

Copy link
Copy Markdown
Owner

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.

@supersmile2009

Copy link
Copy Markdown
Contributor Author

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.

@qlty-cloud-legacy

Copy link
Copy Markdown

Code Climate has analyzed commit 2cc9a70 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.

@k911

k911 commented Jan 28, 2021

Copy link
Copy Markdown
Owner

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

->defaultNull()
->setDeprecated(
'k911/swoole-bundle',
'0.8.4',

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I can change that after merge, however it'll be at least 0.9.0 version. So just a note for me.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants