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

CiviMail: If SMTP connection error is detected, disconnect so we can reconnect and retry #11840

Merged
merged 1 commit into from
Jun 8, 2018

Conversation

mfb
Copy link
Contributor

@mfb mfb commented Mar 20, 2018

Overview

If an SMTP connection error is detected, then we should disconnect so that a reconnect is possible. In addition, we'll need to retry any deliveries that had an SMTP error later on. Fixes https://issues.civicrm.org/jira/browse/CRM-20320

Before

Currently, if there is an SMTP timeout, multiple errors are logged (initially code: 421, response: Timeout waiting for data from client; followed by SMTP: Failed to write to socket: unknown error). After 6 such errors, CiviMail gives up sending mail.

After

When an SMTP connection error is detected, CiviMail should disconnect the SMTP connection so that a reconnect is possible on the next attempt. We also need to record the delivery group as not completed so it can be retried later.

Technical Details

With this change, Mailer Batch Limit can be set to 0, allowing mailings to be delivered continously as fast as possible, rather than stopping with an error condition and waiting for a new cron job to start.

… reconnect, and retry the group later to complete delivery
@mfb mfb force-pushed the smtp-reconnect branch from 1d5023e to e865420 Compare March 20, 2018 11:01
@mfb mfb changed the title if SMTP connection error is detected, disconnect so we can reconnect if SMTP connection error is detected, disconnect so we can reconnect (and retry the partially failed delivery group) Mar 20, 2018
@mfb mfb changed the title if SMTP connection error is detected, disconnect so we can reconnect (and retry the partially failed delivery group) CiviMail: If SMTP connection error is detected, disconnect so we can reconnect and retry Mar 24, 2018
@JKingsnorth
Copy link
Contributor

We have reviewed this and will be implementing it on our site. It seems like a sound improvement. The only issue is if a mailing job repeatedly fails on this, so the mailing job never gets marked as completed, as outlined in https://issues.civicrm.org/jira/browse/CRM-20320

Similar patch has also been merged into flexmailer, so let's merge this here too.

@mfb
Copy link
Contributor Author

mfb commented Jun 8, 2018

We've been running this in production for some time and it's working great, on 3 different mail servers, not just AWS.

IMHO if message delivery to your mail service fails in a temporary manner, then it makes sense that delivery is reattempted later. I can think of lots of reasons that should be the case and no reason it shouldn't be the case :) If, for example, your mail service null-routed your IP address rather than simply rejecting your messages, then trying to send again later wouldn't really make the situation any worse; and once you resolve the problem, delivery would be completed automatically.

@eileenmcnaughton
Copy link
Contributor

Looking at the risk profile of this patch

  1. it is very hard to test or unit test HOWEVER
  2. it ONLY kicks in when there is an identified connection problem so it is only intervening when the flow is already in jeopardy and
  3. it has had good production testing per above
  4. similar patch has been merged to flexmailer per @JKingsnorth

On balance I am going to merge this - although I do note it is probably possible and better for people to switch to flexmailer than patch core

@eileenmcnaughton eileenmcnaughton merged commit eaf537d into civicrm:master Jun 8, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants