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

[Messenger] Break application if the database is not UP #37069

Closed
lyrixx opened this issue Jun 2, 2020 · 13 comments
Closed

[Messenger] Break application if the database is not UP #37069

lyrixx opened this issue Jun 2, 2020 · 13 comments

Comments

@lyrixx
Copy link
Member

lyrixx commented Jun 2, 2020

Symfony version(s) affected: 5.1.0

Description

When symfony boot (CLI), the messenger stack is instanciated. And so the transports are instanciated. Then the doctrine transport tries to connect to the DB. And if there are no DB (in CI for example) it crashs !

It was not the case in 4.4.*

We have the following code:

if ($useNotify && ($wrappedConnection = $driverConnection->getWrappedConnection()) && method_exists($wrappedConnection, 'pgsqlGetNotify')) {
$connection = new PostgreSqlConnection($configuration, $driverConnection);
} else {
$connection = new Connection($configuration, $driverConnection);
}

This part of the code $driverConnection->getWrappedConnection() calls
https://github.com/doctrine/dbal/blob/18a053ad016207a66937f78f367f900c771aed94/lib/Doctrine/DBAL/Connection.php#L1455-L1460

And so the connection is established. So if the DB server is not available, it crashed.

In my case, it occrus during PHPStan test suite. In that case, we don't have a DB setup'ed so it does not works.

BTW, it occurs on every single command because messenger try load theses information when symfony boot 😬

Here is the full stack trace:

In PDOConnection.php line 27:
                                                                                               
  [PDOException]                                                                               
  PDO::__construct(): php_network_getaddresses: getaddrinfo failed: Name or service not known  
                                                                                               

Exception trace:
  at /home/circleci/redirection.io/server/backend/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:27
 PDO->__construct() at /home/circleci/redirection.io/server/backend/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php:27
 Doctrine\DBAL\Driver\PDOConnection->__construct() at /home/circleci/redirection.io/server/backend/vendor/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOMySql/Driver.php:22
 Doctrine\DBAL\Driver\PDOMySql\Driver->connect() at /home/circleci/redirection.io/server/backend/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:362
 Doctrine\DBAL\Connection->connect() at /home/circleci/redirection.io/server/backend/vendor/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1449
 Doctrine\DBAL\Connection->getWrappedConnection() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Messenger/Bridge/Doctrine/Transport/DoctrineTransportFactory.php:50
 Symfony\Component\Messenger\Bridge\Doctrine\Transport\DoctrineTransportFactory->createTransport() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Messenger/Transport/TransportFactory.php:36
 Symfony\Component\Messenger\Transport\TransportFactory->createTransport() at /home/circleci/redirection.io/server/backend/var/cache/dev/ContainerXfUbn4u/AppKernelDevDebugContainer.php:14822
 ContainerXfUbn4u\AppKernelDevDebugContainer->getMessenger_Transport_FailedService() at /home/circleci/redirection.io/server/backend/var/cache/dev/ContainerXfUbn4u/AppKernelDevDebugContainer.php:11684
 ContainerXfUbn4u\AppKernelDevDebugContainer->getConsole_Command_MessengerFailedMessagesRetryService() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:441
 Symfony\Component\DependencyInjection\Container->getService() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Argument/ServiceLocator.php:40
 Symfony\Component\DependencyInjection\Argument\ServiceLocator->get() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/CommandLoader/ContainerCommandLoader.php:45
 Symfony\Component\Console\CommandLoader\ContainerCommandLoader->get() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:520
 Symfony\Component\Console\Application->has() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:716
 Symfony\Component\Console\Application->all() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:142
 Symfony\Bundle\FrameworkBundle\Console\Application->all() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Descriptor/ApplicationDescription.php:91
 Symfony\Component\Console\Descriptor\ApplicationDescription->inspectApplication() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Descriptor/ApplicationDescription.php:68
 Symfony\Component\Console\Descriptor\ApplicationDescription->getCommands() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Descriptor/TextDescriptor.php:202
 Symfony\Component\Console\Descriptor\TextDescriptor->describeApplication() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Descriptor/Descriptor.php:55
 Symfony\Component\Console\Descriptor\Descriptor->describe() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Helper/DescriptorHelper.php:65
 Symfony\Component\Console\Helper\DescriptorHelper->describe() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Command/ListCommand.php:75
 Symfony\Component\Console\Command\ListCommand->execute() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php:258
 Symfony\Component\Console\Command\Command->run() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:929
 Symfony\Component\Console\Application->doRunCommand() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:99
 Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:264
 Symfony\Component\Console\Application->doRun() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:82
 Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at /home/circleci/redirection.io/server/backend/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:140
 Symfony\Component\Console\Application->run() at /home/circleci/redirection.io/server/backend/bin/console:29

list [--raw] [--format FORMAT] [-h|--help] [-q|--quiet] [-v|vv|vvv|--verbose] [-V|--version] [--ansi] [--no-ansi] [-n|--no-interaction] [-e|--env ENV] [--no-debug] [--] [<command> [<namespace>]]
@fbourigault
Copy link
Contributor

I have the same issue and same conclusion.

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 5, 2020

From my experience, Symfony is quite eager to load services when that wasn't expected. The same issue happened with SNC Redis having no Redis server when building Docker images. The solution there was marking the service as lazy. /cc @B-Galati

Same here: Algatux/influxdb-bundle#82

This seems like a systemic problem, TBH.

@stof
Copy link
Member

stof commented Oct 5, 2020

The issue is the usage of APIs triggering the DB connection in the transport factory. The factory should not require connecting to the DB.

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 5, 2020

@stof could this be solved by adding a tag to specific services and triggering them in boot / cache warmup as an error?

To be exact, since the difference between "build" and "cache" was introduced in #36515, there obviously exists "build-time" and "run-time", accessing runtime-only services at build-time should be considered an error. Marking Doctrine connection as such would highlight this issue during development.

@stof
Copy link
Member

stof commented Oct 5, 2020

well, here, this should be solved by avoiding to connect to the database in the DoctrineTransportFactory. It should not be calling $driverConnection->getWrappedConnection() as this requires connecting to the DB to make this wrapped connection exist.

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 5, 2020

Discussion from Slack:

  1. introduce a concept of a build-time container
  2. introduce a concept of marking services (or even individual methods on the services) as "runtime-only"
  3. when in build-time, replace the injected services with mocks, having the runtime-only methods rigged to throw exceptions
  4. this applies to mandatory cache warmers since they're also build-time (building the app, not the container)

This instrumentation would mean any runtime-only interaction during build-time would result in an exception.

@B-Galati
Copy link
Contributor

B-Galati commented Oct 5, 2020

Is it wrong to say that lazy services are made for that sort of cases?

@Guikingone
Copy link
Contributor

Guikingone commented Oct 6, 2020

@dkarlovi IMO, there could be a simpler approach: Moving the connection call on the Connection itself rather than in the factory.

IMO the connection could check each time that a query is executed if the database is up (by default, Doctrine call getWrappedConnection() each time that a call use executeQuery() so there's already a solution?).

This way, if the database/connection is not up, the connection method call fails and not the connection instantiation.

Plus, I agree with @B-Galati that lazy services are already there for solving the concepts that you've listed.

@stof
Copy link
Member

stof commented Oct 6, 2020

@Guikingone getWrappedConnection is precisely meant to access the underlying PDO connection (or other things for non-PDO drivers). DBAL cannot wrap that in another lazy-connecting API, as this method is precisely the one exiting from the DBAL lazy-connecting API. The DBAL API itself already connects lazily.

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 6, 2020

@Guikingone what you describe is solving the consequence, what I'm describing is an attempt to solve the underlying systemic issue which is obvious by the fact the same problem appears over and over. Adding support to recognize it would ensure this stops happening since the framework and bundle maintainers would get an early signal this is happening and adjust their code.

Even if the solution is to make it lazy, you need to know you need to make it lazy to begin with.

@stof
Copy link
Member

stof commented Oct 6, 2020

Making a service lazy won't solve the issue if you call one of the methods on it, which will trigger the initialization of the proxy. The workaround using lazy is to apply it on the service which performs logic in its instantiation, not on the service connecting to an external resource in its usage. And the proper fix here is to remove such usage from the instantiation. Applying a proxy is only a workaround.

@dkarlovi
Copy link
Contributor

dkarlovi commented Oct 6, 2020

the proper fix here is to remove such usage from the instantiation

Correct, there's no panacea here, which is why my suggestion doesn't attempt to find it, it attempts to warn about some sort of a solution being required, the solution would then be done on a case by case basis, but you need to know you need a solution to begin with.

@chalasr
Copy link
Member

chalasr commented Oct 18, 2020

See #38618

@fabpot fabpot closed this as completed Oct 19, 2020
fabpot added a commit that referenced this issue Oct 19, 2020
…ction (chalasr)

This PR was merged into the 5.1 branch.

Discussion
----------

[Messenger][Doctrine] Avoid early db access for pgsql detection

| Q             | A
| ------------- | ---
| Branch?       | 5.1
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #37069
| License       | MIT
| Doc PR        | -

Keeps the connection lazy using an instanceof check on the DBAL driver instead.

Commits
-------

c4cc4a3 [Messenger][Doctrine] Avoid early db access for pgsql detection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests