Skip to content

[Mailer] Prevent MessageLoggerListener from leaking in env=prod #37712

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.

Sign up for GitHub

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

Jump to bottom
Merged
fabpot merged 1 commit into from
Aug 2, 2020
Merged

[Mailer] Prevent MessageLoggerListener from leaking in env=prod #37712

fabpot merged 1 commit into from
Aug 2, 2020

Conversation

@vudaltsov
Copy link
Contributor

@vudaltsov vudaltsov commented Jul 31, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? yes
License MIT

I was trying to send a batch of emails with --env=prod when I noticed that MessageLoggerListener was still collecting all the messages and leaking the memory. I tried to do $this->getApplication()->reset(), but it didn't work because MessageLoggerListener was not tagged (now fixed in #37705).

In this PR I propose to move the declaration of MessageLoggerListener to mailer_debug.php since the only service depending on it is mailer.data_collector from mailer_debug.php. If a developer needs to collect sent emails, a custom listener could be implemented on the project side.

  • deprecate the service
  • add a new one to mailer_debug.php
  • add a line to CHANGELOG.md
  • add a line to UPGRADE-5.2.md

@fabpot
Copy link
Member

fabpot commented Jul 31, 2020

What's the problem keeping it in prod (now that it does not leak memory anymore)?

@vudaltsov
Copy link
Contributor Author

vudaltsov commented Jul 31, 2020

At first I had a hypothesis that MessageLoggerListener was declared in mailer.php(xml) by mistake.

If that's not the case, then why it is there when no other service depends on it? Obviously DI cannot automatically remove mailer.logger_message_listener, since it is a listener. But in fact it's just collecting information in prod for nothing.

And yes, it's still leaking without calling $serviceResetter->reset()! Before installing Symfony Mailer nothing was leaking in my worker in prod. Now I have to add a worker listener to do $serviceResetter->reset() just because of the MessageLoggerListener which I don't use.

I also found a comment by @nicolas-grekas :

having a non-leaking core is very important to me

This PR can help to accomplish this goal 😉

@fabpot
Copy link
Member

fabpot commented Jul 31, 2020

Not using something in core does not mean that we should remove it.
But anyway, I don't have any problem with moving it to debug.

@nicolas-grekas nicolas-grekas added this to the next milestone Aug 1, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Aug 1, 2020

(please mind the test failure)

@fabpot fabpot force-pushed the mailer-logger-message-listener-debug branch from dadb3fb to e226775 Compare August 2, 2020 07:59
@fabpot
Copy link
Member

fabpot commented Aug 2, 2020

Thank you @vudaltsov.

@fabpot fabpot mentioned this pull request Aug 2, 2020
Closed
@fabpot fabpot merged commit 1889ba8 into symfony:master Aug 2, 2020
@vudaltsov vudaltsov deleted the mailer-logger-message-listener-debug branch August 2, 2020 14:33
@vudaltsov
Copy link
Contributor Author

vudaltsov commented Aug 2, 2020

@fabpot , should I add a changelog entry?

@nicolas-grekas nicolas-grekas modified the milestones: next, 5.2 Oct 5, 2020
@fabpot fabpot mentioned this pull request Oct 5, 2020
Merged
@alexkingdom alexkingdom mentioned this pull request Jan 28, 2022
Closed
@izayoi256 izayoi256 mentioned this pull request Jun 2, 2023
Open
@cmodijk cmodijk mentioned this pull request Jun 6, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees

No one assigned

Labels

Deprecation Feature Mailer Status: Reviewed

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

4 participants

@vudaltsov @fabpot @nicolas-grekas @carsonbot