long-running processes: allow checking the database connection

[alli83]

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

[greg0ire]

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.

[nicolas-grekas]

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

[derrabus]

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]

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]

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]

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

No.

[alli83]

@derrabus ok I'll update it!

[greg0ire]

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.

[dunglas]

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

[dunglas]

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]

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]

@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]

@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]

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]

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]

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]

symfony/symfony#53214 for listener

[greg0ire]

@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]

symfony/symfony#53214 for listener

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

[greg0ire]

symfony/symfony#53214 for listener

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