Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[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.

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

Conversation

@vudaltsov
Copy link
Contributor

@vudaltsovvudaltsov commentedJul 31, 2020•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
Deprecations?yes
LicenseMIT

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

In this PR I propose to move the declaration ofMessageLoggerListener tomailer_debug.php since the only service depending on it ismailer.data_collector frommailer_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 tomailer_debug.php
  • add a line to CHANGELOG.md
  • add a line to UPGRADE-5.2.md

@fabpot
Copy link
Member

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

@vudaltsov
Copy link
ContributorAuthor

At first I had a hypothesis thatMessageLoggerListener was declared inmailer.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 removemailer.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 theMessageLoggerListener which I don't use.

I also founda comment by@nicolas-grekas :

having a non-leaking core is very important to me

This PR can help to accomplish this goal 😉

vkondratjevs reacted with thumbs up emoji

@fabpot
Copy link
Member

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-grekasnicolas-grekas added this to thenext milestoneAug 1, 2020
@nicolas-grekas
Copy link
Member

(please mind the test failure)

@fabpotfabpotforce-pushed themailer-logger-message-listener-debug branch fromdadb3fb toe226775CompareAugust 2, 2020 07:59
@fabpot
Copy link
Member

Thank you@vudaltsov.

vkondratjevs reacted with thumbs up emojivudaltsov reacted with hooray emoji

@fabpotfabpot merged commit1889ba8 intosymfony:masterAug 2, 2020
@vudaltsovvudaltsov deleted the mailer-logger-message-listener-debug branchAugust 2, 2020 14:33
@vudaltsov
Copy link
ContributorAuthor

@fabpot , should I add a changelog entry?

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.2

Development

Successfully merging this pull request may close these issues.

4 participants

@vudaltsov@fabpot@nicolas-grekas@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp