-
Notifications
You must be signed in to change notification settings - Fork 333
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
Clean up send error handling; fix stalled send queue. #78
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,8 +12,7 @@ | |
from django.core.mail.message import make_msgid | ||
|
||
from mailer.models import ( | ||
Message, MessageLog, RESULT_SUCCESS, RESULT_FAILURE, get_message_id, | ||
) | ||
Message, MessageLog, RESULT_SUCCESS, RESULT_FAILURE, RESULT_ERROR, get_message_id) | ||
|
||
|
||
# when queue is empty, how long to wait (in seconds) before checking again | ||
|
@@ -103,7 +102,7 @@ def release_lock(lock): | |
logging.debug("released.") | ||
|
||
|
||
def send_all(): | ||
def send_all(): # noqa: C901 # TODO: refactor to reduce complexity | ||
""" | ||
Send all eligible messages in the queue. | ||
""" | ||
|
@@ -122,6 +121,7 @@ def send_all(): | |
start_time = time.time() | ||
|
||
deferred = 0 | ||
errored = 0 | ||
sent = 0 | ||
|
||
try: | ||
|
@@ -154,19 +154,35 @@ def send_all(): | |
logging.warning("message discarded due to failure in converting from DB. Added on '%s' with priority '%s'" % (message.when_added, message.priority)) # noqa | ||
message.delete() | ||
|
||
except (socket_error, smtplib.SMTPSenderRefused, | ||
smtplib.SMTPRecipientsRefused, | ||
smtplib.SMTPDataError, | ||
smtplib.SMTPAuthenticationError) as err: | ||
except (smtplib.SMTPConnectError, | ||
smtplib.SMTPServerDisconnected) as err: | ||
# Connection-related problem -- try again soon: | ||
logging.info("message left queued due to transient failure: %s" % err) | ||
MessageLog.objects.log(message, RESULT_FAILURE, log_message=str(err)) | ||
break # connectivity issue; resume batch next time | ||
|
||
except (socket_error, # includes smtplib.SMTPException in python3.4+ | ||
# TODO: consider handling socket_error/OSError/IOError as "retry soon"??? | ||
smtplib.SMTPException) as err: | ||
# Destination/recipient-related problem -- try again later: | ||
message.defer() | ||
logging.info("message deferred due to failure: %s" % err) | ||
MessageLog.objects.log(message, RESULT_FAILURE, log_message=str(err)) | ||
deferred += 1 | ||
# Get new connection, it case the connection itself has an error. | ||
connection = None | ||
|
||
except Exception as err: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New to the party but why are you deleting these instead of deferring them too? Seems like a harsh thing to force on people not using SMTPlibs. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the "deferring permanent errors isn't helpful, and can lead to other problems" case discussed in #73. IIRC, one of the reasons I didn't follow through with this PR is that it's not at all clear how to reliably distinguish "transient error, should defer and retry later" from "permanent error, retry won't be helpful". More discussion in #73. If you have an idea for an approach that would work well with a variety of email backends, it'd be awesome to have someone adopt this PR and drive it to completion. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, missed the "other problems" and that is indeed a concern. Thanks for the update, I had read that you were stepping away from the project so I appreciate the comment. |
||
# Content-specific (or unknown) problem -- don't retry this message: | ||
logging.info("message discarded due to error: %s" % err) | ||
MessageLog.objects.log(message, RESULT_ERROR, log_message=str(err)) | ||
message.delete() | ||
errored += 1 | ||
# Get new connection, it case the connection itself has an error. | ||
connection = None | ||
|
||
# Check if we reached the limits for the current run | ||
if _limits_reached(sent, deferred): | ||
if _limits_reached(sent, deferred): # TODO: add limits for errored??? | ||
break | ||
|
||
_throttle_emails() | ||
|
@@ -175,7 +191,7 @@ def send_all(): | |
release_lock(lock) | ||
|
||
logging.info("") | ||
logging.info("%s sent; %s deferred;" % (sent, deferred)) | ||
logging.info("%s sent; %s deferred; %s errored" % (sent, deferred, errored)) | ||
logging.info("done in %.2f seconds" % (time.time() - start_time)) | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm personally not that worried about complexity here - I don't think clarity would be increased that much by refactoring it.