[Messenger] Allow to close the transport connection #59862
Conversation
433dad7 to
5122929
Compare
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 to
edaf24a
Compare
|
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) |
| @@ -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.
this transport does not own the DBAL connection, so it should not close it either
There was a problem hiding this comment.
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.
@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.
DoctrineCloseConnectionMiddleware is closing the connection if 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)
src/Symfony/Component/Messenger/Transport/CloseableTransportInterface.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Messenger/Transport/CloseableTransportInterface.php
Outdated
Show resolved
Hide resolved
2ece7af to
7e24e54
Compare
|
status:needs review |
28f85d4 to
9788aee
Compare
|
Thank you @andrew-demb. |
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)