Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
[Messenger] Allow to close the transport connection#59862
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
433dad7 to5122929Compare
I don't know how to handle this error. I didn't add Relay in the PR - probably an issue introduced by the Psalm update or existed before. |
This CI failure for PHP 8.5 seems unrelated to PR |
At first glance, this looks like something that should be handled via a middleware, something like |
@HypeMC my use-case - a long-running webserver. Some of the requests trigger If such a request is handled - Redis connection is not closed until the webserver worker will be restarted. |
is it an issue that the Redis connection is not closed ? When using a long-running webserver instead of PHP-FPM, isn't one of the goals being able to keep such connections instead of reconnecting all the time ? |
@stof As we're used (in our project) Redis for queues not so often, we're trying to avoid additional connections being opened to Redis with no reason (and other "cold" storages). Just checked how Symfony behaves with Doctrine DBAL connections - they are not closed between requests either. I'm OK with changing the PR implementation just to open the way to close connection (add method "close" to the Redis transport) - to allow close connection from the application code (using the event system). |
doctrine can be optionally closed at the middelware layer:https://github.com/symfony/symfony/blob/7.3/src/Symfony/Bridge/Doctrine/Messenger/DoctrineCloseConnectionMiddleware.php im wondering if redis should follow a similar approach |
I can introduce the interface "CloseableTransportInterface" that will allow closing used transport connections from the userland (in my case - from "kernel.terminate" event listener). I can see, that this can be implemented by most transports (AMQP, Redis, Doctrine DBAL, etc) |
619bffb toedaf24aCompareAccording to the feedback, I changed the implementation to another one to help with our use case. Implemented a way to close messenger transport from the application, to allow free resources for long-running processes (like a long-running webserver) |
| @@ -80,6 +80,12 @@ public function reset(): void | |||
| $this->doMysqlCleanup = false; | |||
| } | |||
| public function close(): void | |||
| { | |||
| $this->driverConnection->close(); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
this transport does not own the DBAL connection, so it should not close it either
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Updated. Other transports seem OK to me to be closeable (they remain their own connection or already have some "reset" methods).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@stof from a messenger:consume POV i think doctrine transport is "closeable"
that's what DoctrineCloseConnectionMiddleware does no?
i tend to believe we should either solve it at the transport layer or the middleware layer in an agnostic matter IMHO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
DoctrineCloseConnectionMiddleware is closing the connectionif you enable that middleware.
the transport itself should not close the connection implicitly, because it is not owning the connection (the connection might be used for multiple purposes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Can you add a note in the CHANGELOG file about the new interface?
src/Symfony/Component/Messenger/Transport/CloseableTransportInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Messenger/Transport/CloseableTransportInterface.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
2ece7af to7e24e54Comparestatus:needs review |
28f85d4 to9788aeeCompareThank you@andrew-demb. |
89bae9c intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
1. Implemented the possibility to make messenger transports resettable2. Implemented reset Redis connection for Redis messenger transportThis feature may lead to decreased performance for messenger consumers (because connection will be resetted between processing messages).One way to resolve it that I found - add configuration for resettable transports - aka "reset connection on kernel reset: true/false". For consumers it can be configured via env var to be "false", but for web "true".UPD 2025-02-27: According to the feedback, I changed the implementation to another one to help with our use case.
Implemented a way to close messenger transport from the application, to allow free resources for long-running processes (like a long-running webserver)