[Mesenger] Add support for reseting container services between 2 messages #41163

lyrixx · 2021-05-10T15:55:34Z

Q	A
Branch?	5.4
Bug fix?	no
New feature?	yes
Deprecations?	no
Tickets
License	MIT
Doc PR	symfony/symfony-docs#15796

Without this patch, services are not resetted. For example Monolog
Finger Cross handler is never reset nor flushed. So if the first
message trigger and "error" level message, all others message will log
and overflow the buffer.

So, when a transport is async (it means it is run in a worker), it's highly preferable to this configuration on

Usage with framework:

framework:
    messenger:
        transports:
            async:
                dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
                reset_on_message: true
            failed: 'doctrine://default?queue_name=failed'
            sync: 'sync://'

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

lyrixx · 2021-07-23T09:18:44Z

Hello, I have rebased this PR, and addressed all comments. I should be OK now

lyrixx · 2021-09-01T08:16:14Z

I could rebase this PR, but before doing so, I prefer to be sure it's mergeable. so WDYT?

Edit: I have rebased the PR

Jeroeny · 2021-09-03T15:18:51Z

So if the first message trigger and "error" level message, all others message will log and overflow the buffer.

From my experience, a worker running without processing messages can also go out of memory. For example if you have an app running in debug mode the tracing/profiling will keep event-dispatcher events in memory, eventually causing the process to go out of memory.

Because of this, should there also be a possibility to trigger the reset onWorkerRunning instead of just the WorkerMessageHandledEvent and WorkerMessageFailedEvent ?

And continuing on that, would it be nice if you could do a reset when the worker is inactive (Can be read WorkerRunningEvent->isWorkerIdle()) ? . So that if your reset functionality has a bit of performance impact, it could have less impact on the message handling performance.

lyrixx · 2021-09-03T15:25:30Z

From my experience, a worker running without processing messages can also go out of memory. For example if you have an app running in debug mode the tracing/profiling will keep event-dispatcher events in memory, eventually causing the process to go out of memory.

Indeed, cf #32422

Because of this, should there also be a possibility to trigger the reset onWorkerRunning instead of just the WorkerMessageHandledEvent and WorkerMessageFailedEvent ?

I don't understand :/

And continuing on that, would it be nice if you could do a reset when the worker is inactive (Can be read WorkerRunningEvent->isWorkerIdle()) ? . So that if your reset functionality has a bit of performance impact, it could have less impact on the message handling performance.

Indeed, that would be possible, in a next PR :)

Jeroeny · 2021-09-03T15:28:11Z

If I understood the logic in this PR correctly, a reset is only triggered by on of those WorkerMessage.. events right? So it's only going to reset if messages are being consumed. What about a worker/consumer that's idle for a while? It could go out of memory for the same reasons and might benefit from reset of its services.

lyrixx · 2021-09-03T15:31:36Z

Ohhh, I see, good point. I need to check when the running event is triggered to ensure it does not break something

Jeroeny · 2021-09-03T15:37:02Z

when the running event is triggered

I think it should be just as safe as the other events, it at least not during the handling of a message:

https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Messenger/Worker.php#L95
https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/Messenger/Worker.php#L111

lyrixx · 2021-09-06T15:35:28Z

@Jeroeny Here we go 👍🏼 I pushed a new version where the listener listen WorkerRunningEvent too. Thanks for the hint 👍🏼

fabpot

Can you update the description to explain when and why one should enable or disable this flag? If you can create a PR on the docs, that would be the cherry on the cake :)

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

…ssenger message. Without this patch, services are not resetted. For example Monolog Finger Cross handler is never reset nor flushed. So if the first message trigger and "error" level message, all others message will log and overflow the buffer. Usage with framework: ```yaml framework: messenger: transports: async: dsn: '%env(MESSENGER_TRANSPORT_DSN)%' reset_on_message: true failed: 'doctrine://default?queue_name=failed' sync: 'sync://' ```

lyrixx · 2021-09-10T09:15:34Z

@fabpot I haved edited the PR description, and the configuration.
I'll open the doc PR asap (edit: symfony/symfony-docs#15796)

fabpot · 2021-09-10T09:42:18Z

Thank you @lyrixx.

stefangr · 2021-09-12T09:02:24Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/messenger.php

@@ -195,6 +196,12 @@
            ->tag('kernel.event_subscriber')

        ->set('messenger.listener.stop_worker_on_stop_exception_listener', StopWorkerOnCustomStopExceptionListener::class)
+


Is it correct that the StopWorkerOnCustomStopExceptionListener service looses the kernel.event_subscriber tag?

Thanks, fixed in #43002

…rkerOnCustomStopExceptionListener (lyrixx) This PR was merged into the 5.4 branch. Discussion ---------- [Messenger] Add back kernel.event_subscriber tag on StopWorkerOnCustomStopExceptionListener | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes (but the bug was not released) | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | refs: #41163 (comment) Commits ------- 00a34c6 [Messenger] Add back kernel.event_subscriber tag on StopWorkerOnCustomStopExceptionListener

…rkerOnCustomStopExceptionListener (lyrixx) This PR was merged into the 5.4 branch. Discussion ---------- [Messenger] Add back kernel.event_subscriber tag on StopWorkerOnCustomStopExceptionListener | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes (but the bug was not released) | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | refs: symfony/symfony#41163 (comment) Commits ------- 00a34c623b [Messenger] Add back kernel.event_subscriber tag on StopWorkerOnCustomStopExceptionListener

…lyrixx) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [Messenger] document reset_on_message transport option refs symfony/symfony#41163 Commits ------- 3f7ebce [Messenger] document reset_on_message transport option

lyrixx requested a review from sroze as a code owner May 10, 2021 15:55

carsonbot added Status: Needs Review Feature labels May 10, 2021

nicolas-grekas added this to the 5.x milestone May 10, 2021

lyrixx force-pushed the worker-reset-on-message branch from 0a51dc6 to ae3dc21 Compare May 10, 2021 18:15

This comment has been minimized.

Sign in to view

fabpot requested changes Jul 3, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 3, 2021

lyrixx force-pushed the worker-reset-on-message branch from ae3dc21 to d9b324a Compare July 23, 2021 09:17

lyrixx mentioned this pull request Jul 30, 2021

Register channels to ResetLoggersWorkerSubscriber symfony/monolog-bundle#403

Closed

lyrixx force-pushed the worker-reset-on-message branch from d9b324a to c52c434 Compare September 3, 2021 12:49

lyrixx force-pushed the worker-reset-on-message branch from c52c434 to f0fbce3 Compare September 6, 2021 15:34

lyrixx requested a review from fabpot September 9, 2021 08:15

carsonbot added Status: Needs Review and removed Status: Needs Work labels Sep 9, 2021

lyrixx requested review from nicolas-grekas, chalasr and dunglas September 9, 2021 08:16

fabpot requested changes Sep 10, 2021

View reviewed changes

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php Outdated Show resolved Hide resolved

carsonbot added Status: Needs Work and removed Status: Needs Review labels Sep 10, 2021

lyrixx force-pushed the worker-reset-on-message branch from f0fbce3 to 488bb88 Compare September 10, 2021 09:13

lyrixx mentioned this pull request Sep 10, 2021

[Messenger] document reset_on_message transport option symfony/symfony-docs#15796

Merged

fabpot approved these changes Sep 10, 2021

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Sep 10, 2021

fabpot merged commit ba7f746 into symfony:5.4 Sep 10, 2021

lyrixx deleted the worker-reset-on-message branch September 10, 2021 09:45

stefangr reviewed Sep 12, 2021

View reviewed changes

ovrflo mentioned this pull request Sep 13, 2021

[RFC][Mesenger] Add support for reseting container services between 2 messages #43001

Closed

lyrixx mentioned this pull request Sep 13, 2021

[Messenger] Add back kernel.event_subscriber tag on StopWorkerOnCustomStopExceptionListener #43002

Merged

upyx mentioned this pull request Sep 20, 2021

Too early resetting services in messenger #43114

Closed

lyrixx mentioned this pull request Oct 1, 2021

[RFC][Messenger] Remove ResetLoggersWorkerSubscriber #43271

Closed

Jeroeny mentioned this pull request Oct 2, 2021

[HttpClient] Fix memory leak in TraceableHttpClient for environments with profiling #43287

Closed

nicolas-grekas mentioned this pull request Nov 4, 2021

[HttpClient] Dns cache issue in long running process #43929

Closed

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Mesenger] Add support for reseting container services between 2 messages #41163

[Mesenger] Add support for reseting container services between 2 messages #41163

lyrixx commented May 10, 2021 •

edited

Loading

This comment has been minimized.

lyrixx commented Jul 23, 2021

lyrixx commented Sep 1, 2021 •

edited

Loading

Jeroeny commented Sep 3, 2021 •

edited

Loading

lyrixx commented Sep 3, 2021

Jeroeny commented Sep 3, 2021

lyrixx commented Sep 3, 2021

Jeroeny commented Sep 3, 2021

lyrixx commented Sep 6, 2021

fabpot left a comment

lyrixx commented Sep 10, 2021 •

edited

Loading

fabpot commented Sep 10, 2021

stefangr Sep 12, 2021

lyrixx Sep 13, 2021

		@@ -195,6 +196,12 @@
		->tag('kernel.event_subscriber')

		->set('messenger.listener.stop_worker_on_stop_exception_listener', StopWorkerOnCustomStopExceptionListener::class)

[Mesenger] Add support for reseting container services between 2 messages #41163

[Mesenger] Add support for reseting container services between 2 messages #41163

Conversation

lyrixx commented May 10, 2021 • edited Loading

This comment has been minimized.

lyrixx commented Jul 23, 2021

lyrixx commented Sep 1, 2021 • edited Loading

Jeroeny commented Sep 3, 2021 • edited Loading

lyrixx commented Sep 3, 2021

Jeroeny commented Sep 3, 2021

lyrixx commented Sep 3, 2021

Jeroeny commented Sep 3, 2021

lyrixx commented Sep 6, 2021

fabpot left a comment

Choose a reason for hiding this comment

lyrixx commented Sep 10, 2021 • edited Loading

fabpot commented Sep 10, 2021

stefangr Sep 12, 2021

Choose a reason for hiding this comment

lyrixx Sep 13, 2021

Choose a reason for hiding this comment

lyrixx commented May 10, 2021 •

edited

Loading

lyrixx commented Sep 1, 2021 •

edited

Loading

Jeroeny commented Sep 3, 2021 •

edited

Loading

lyrixx commented Sep 10, 2021 •

edited

Loading