Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Mailer] Do not ping the SMTP server before sending every message #35633

Merged
merged 1 commit into from Feb 7, 2020
Merged

[Mailer] Do not ping the SMTP server before sending every message #35633

merged 1 commit into from Feb 7, 2020

Conversation

micheh
Copy link
Contributor

@micheh micheh commented Feb 7, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #35515
License MIT

This pull request changes the SMTP transport to only ping the server if the last message was sent more than a specified number of seconds ago (instead of pinging the server before every message). By default, it will ping the server if 100 or more seconds since the last message have passed.

This should make sending emails with the SMTP transport more robust with many emails, as SMTP servers will often drop the connection if too many non-mail commands are sent (like pinging the server with NOOP commands).

@@ -179,6 +207,8 @@ protected function doSend(SentMessage $message): void
$this->stream->flush();
$this->executeCommand("\r\n.\r\n", [250]);
$message->appendDebug($this->stream->getDebug());

$this->lastSent = microtime(true);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You only update lastSent when a message has been sent. In case of an error, it won't be reset. By reading the issue, I would have expected that lastSent would be the last time we sent a ping.

Copy link
Contributor Author

@micheh micheh Feb 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because the variable should represent the time when the last message was sent. I chose the name to be consistent with the parent AbstractTransport. Do you want me to change it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But if sending the email does not work for some reason, you will ping the server next time you try, not sure if that's what we want.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a strong opinion about this, so I changed it. I updated the name of the field as well, since it now has a slightly different meaning then the one in the AbstractTransport.

@nicolas-grekas nicolas-grekas added this to the 4.4 milestone Feb 7, 2020
@fabpot
Copy link
Member

fabpot commented Feb 7, 2020

Thank you @micheh.

fabpot added a commit that referenced this pull request Feb 7, 2020
…message (micheh)

This PR was squashed before being merged into the 4.4 branch (closes #35633).

Discussion
----------

[Mailer] Do not ping the SMTP server before sending every message

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #35515
| License       | MIT

This pull request changes the SMTP transport to only ping the server if the last message was sent more than a specified number of seconds ago (instead of pinging the server before every message). By default, it will ping the server if 100 or more seconds since the last message have passed.

This should make sending emails with the SMTP transport more robust with many emails, as SMTP servers will often drop the connection if too many non-mail commands are sent (like pinging the server with NOOP commands).

Commits
-------

2817810 [Mailer] Do not ping the SMTP server before sending every message
@fabpot fabpot merged commit 2817810 into symfony:4.4 Feb 7, 2020
@micheh micheh deleted the ticket_35515 branch February 7, 2020 17:04
This was referenced Feb 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants