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

long-running processes: allow checking the database connection #6351

Closed
wants to merge 1 commit into from

Conversation

alli83
Copy link

@alli83 alli83 commented Apr 4, 2024

Q A
Type new feature
Fixed issues symfony/symfony#51661

Summary

DRAFT:
In long-running processes, allows checking the database connection by pinging it, thus avoiding exceptions. Additionally, it provides the flexibility to configure the frequency of the check to be performed in the case of long-running processes.

As stated in this issue, there is a bug with long-running processes where Doctrine is left in a non-operable state, leading to the issuance of exceptions.
While I acknowledge that this approach appears to resolve the problem, the current code has a strong dependency on Symfony. This is why the PR is kept in draft. If you have any suggestions for improvement and providing an extension point that Symfony can utilize, I am all ears! Or simply I move the code directly from the ConnectionLossAwareHandler(Symfony\Bridge) into the Doctrine bal.

linked to:
symfony/symfony#53214
doctrine/DoctrineBundle#1739

@alli83 alli83 marked this pull request as draft April 4, 2024 12:56
@alli83 alli83 changed the title long-running processes: allows checking the database connection long-running processes: allow checking the database connection Apr 4, 2024
src/Connection.php Outdated Show resolved Hide resolved
@greg0ire
Copy link
Member

greg0ire commented Apr 4, 2024

Hi! I think the PingableConnection which we had in Doctrine 2 did something similar, and it's been deprecated: #4119 in favor of handling ConnectionLost. close() is called automatically in case the connection is lost, which means (I think) all a long-running process needs to do is probably this:

try
{
    // execute dummy SQL
} catch (ConnectionLost $e) {
    $logger->notice('Looks like we lost the connection there', ['exception' => $e]);
} 

This could be done in a middleware I think. It cannot be done in a middleware because you need access to the DBAL\Connection object.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

This looks like a quite pragmatic and effective strategy, I hope this can make it into the codebase.

src/Connection.php Outdated Show resolved Hide resolved
src/Connection.php Outdated Show resolved Hide resolved
src/Connection.php Outdated Show resolved Hide resolved
src/Connection.php Outdated Show resolved Hide resolved
src/Connection.php Outdated Show resolved Hide resolved
src/Connection.php Outdated Show resolved Hide resolved
@derrabus
Copy link
Member

derrabus commented Apr 5, 2024

Sorry to be the party pooper:

  • 3.8.x is closed for new features. Please target 4.1.x instead.
  • Tests.
  • Why exactly can't we handle this in a middleware? I really would like to avoid polluting the wrapper level with this logic.

@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2024

Why exactly can't we handle this in a middleware? I really would like to avoid polluting the wrapper level with this logic.

Right now, ConnectionLost is handled by calling Connection::close(), and rethrowing the exception. Doing the same in a middleware would require having access to the wrapper Connection, which means a circular dependency. Maybe it's possible to establish that circular dependency by adding a setter method and calling it after the connection object is built. 🤔

I see one issue with doing this though: if the middleware handles ConnectionLost, it means the wrapper connection no longer handles it, and the middleware should itself reproduce the handling logic already present in the wrapper connection (when handling the exception, or when connect()'ing again after a call to close(), such as restarting a transaction when appropriate):

dbal/src/Connection.php

Lines 222 to 224 in c70bae9

if ($this->autoCommit === false) {
$this->beginTransaction();
}

It would mean having to keep the code inside the wrapper connection and inside the middleware carefully in sync.

@alli83 alli83 force-pushed the long-running-runtime-check-db branch from cacec40 to e6459ef Compare April 8, 2024 13:00
@alli83
Copy link
Author

alli83 commented Apr 8, 2024

Thank you all for your feedback. Indeed, I need to add some tests.
@derrabus Wouldn't it be more of a bug fix instead?

@derrabus
Copy link
Member

derrabus commented Apr 8, 2024

Wouldn't it be more of a bug fix instead?

No.

@alli83
Copy link
Author

alli83 commented Apr 8, 2024

@derrabus ok I'll update it!

@alli83 alli83 force-pushed the long-running-runtime-check-db branch from e6459ef to 5a800f0 Compare April 9, 2024 02:06
@alli83 alli83 changed the base branch from 3.8.x to 4.1.x April 9, 2024 02:08
src/Connection.php Outdated Show resolved Hide resolved
@alli83 alli83 force-pushed the long-running-runtime-check-db branch 2 times, most recently from 8450eac to fc382de Compare April 11, 2024 02:03
@alli83 alli83 marked this pull request as ready for review April 11, 2024 02:29
@alli83 alli83 force-pushed the long-running-runtime-check-db branch 3 times, most recently from b80fe31 to d69e0cd Compare April 11, 2024 05:17
… pinging it, thus avoiding exceptions. Additionally, it provides the flexibility to configure the frequency of the check to be performed in the case of long-running processes.
@greg0ire
Copy link
Member

greg0ire commented Apr 11, 2024

Just gave this a bit more thought, and I'm wondering what will happen if this is done only for MySQL… we know the issue exists for PostgreSQL as well: dunglas/frankenphp#438 I would hate it if we started being the cause for MySQL being recommended over other RDBMSes 😁
Are the next steps to take a holistic approach and implement ConnectionLost for all RDBMS where it makes sense to do so?

Also, I'm wondering if a way of addressing the issue would be to let the user specify a session timeout (or maybe even obtain it from the server itself somehow), and forcibly close the connection unconditionally, without any try/catch a few moments before we predict it will time out. I wonder how robust that would be, I'm uneasy basing it on time but I'd like to hear your thoughts on that, because I think it could work for all platforms. In fact, in this MR, if the heartbeat is set close enough to the session timeout, the behavior would be quite close, right?

I don't even know if the concept of session is widespread among RDBMS.

@@ -210,7 +219,23 @@ public function createExpressionBuilder(): ExpressionBuilder
protected function connect(): DriverConnection
{
if ($this->_conn !== null) {
return $this->_conn;
$isTimeToCheck = time() - $this->lastCheckedAt >= $this->heartbeat;
Copy link
Contributor

Choose a reason for hiding this comment

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

micro-optim: wrap this in if ($this->heartbeat !== null) to prevent the useless call to time() if possible.


$isAvailable = $this->reconnectOnFailure();

$this->lastCheckedAt = time();
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe could you store the previous call to time() in a variable to prevent doing two calls, also to prevent weird issues, likely be better to have a time older than the check than younger as in the current implementation.

@greg0ire
Copy link
Member

greg0ire commented Apr 11, 2024

After reading @stof 's comment, I think that this is not the right way either: connect() is called super often, so I think we should keep that method as simple as possible. Anyway, I doubt your intent is really to check this on every call to connect(). The right moment to act IMO is at the beginning of the processing of the message/request (be it an HTTP request, or a RabbitMQ message).

@ostrolucky did not like the Symfony PR because it would result in an SQL request for every message, but again, I don't think you actually have to issue a request at all. Instead you could forcibly close the connection (by calling Connection::close() if the last check is considered too old, it should be a good enough approximation.

Like @stof, I'd really like to know what exact scenario you want to fix here, because according to MySQL's documentation, one has to wait 8 hours (!) before the issue I think you're trying to address appears.

@dunglas
Copy link
Contributor

dunglas commented Apr 11, 2024

@greg0ire When using FrankenPHP, RoadRunner, and the like, it's common that the worker script (and so, if using Symfony, the instance of the Symfony kernel) runs much more longer than 8 hours.
Also, some hosters/cloud platforms use shorter timeouts.

@greg0ire
Copy link
Member

greg0ire commented Apr 12, 2024

@dunglas I don't doubt that, what I'm saying is that it's quite unlikely that a RoadRunner or FrankenPHP worker sends 0 SQL requests in 8 hours. In case it does happen, or in case a shorter timeout is used, closing the connection when 95% of the time is elapsed seems IMO like the right thing to do, and needs IMO to be done once per request. I doubt there's a need to perform this check in the middle of processing a request.

Benefits:

  • no pollution of the wrapper scope
  • the code can be isolated in a dedicated class
  • using close(), the network is not involved at all, unlike when you send a dummy request, so it's OK to perform the check on every HTTP request.
  • it works for every platform

Cons:

  • If the setting is not close to the wait_timeout, we will be wasting sessions (but maybe setting the proper timeout can be automated somehow), so it has to be fine tuned.
  • There has to be some margin, e.g. if the timeout is 30s, it might be safer to do a check every 29s rather than every 29.999 seconds, leading to a bit more "waste".

I'm using quotes around "waste", because I think in both cases, if the timeout is high enough, the waste is negligible.

@nicolas-grekas
Copy link
Member

So, we should have a request listener that does this heartbeat when no connections have been made since a long time?
Would that solve all use cases?

@alli83
Copy link
Author

alli83 commented Apr 12, 2024

Yes indeed, I will add a listener (onKernelRequest) and try to timestamp the last SQL request. Based on the time difference between that last request and the current one, I will proceed to close the connection.

@mrossard
Copy link

what I'm saying is that it's quite unlikely that a RoadRunner or FrankenPHP worker sends 0 SQL requests in 8 hour

I'm using frankenphp on a business app that is - not surprinsingly - only used during business hours. 0 requests for 8 straight hours happens pretty much every night...and I've had to disable worker mode because of this.

@alli83
Copy link
Author

alli83 commented Apr 16, 2024

symfony/symfony#53214 for listener

@greg0ire
Copy link
Member

@mrossard then maybe the adjective I should have used is "infrequent". My point is, the code that addresses this issue should not live in what might not be the hot path, but is probably quite warm already.

@mrossard
Copy link

mrossard commented Apr 16, 2024

symfony/symfony#53214 for listener

Thanks, I'd just found it. I'll try this on my staging environment!

@greg0ire
Copy link
Member

symfony/symfony#53214 for listener

I think this approach is much better as the code is isolated and simple :)

@alli83 alli83 closed this Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants