Clean up send error handling; fix stalled send queue. #78
Conversation
[NOT FOR MERGE yet: see "TODO" comments for discussion] In engine.send_all: * Handle transient connection issues by logging failure and cleanly exiting send_all. (Previously, crashed without logging.) * Handle all other smtplib.SMTPException by deferring message. (Previously, behavior differed between Python 2 and 3.) * Handle all other exceptions by logging error with new RESULT_ERROR status and deleting message from send queue. (Previously, crashed without logging, leaving possibly-unsendable message blocking send queue.) **Potentially-breaking change:** If you are using a MAILER_EMAIL_BACKEND other than the default SMTP EmailBackend, errors from that backend *may* be handled differently now. Addresses pinax#73. (And additional discussion there.)
|
Some initial feedback: First, thanks so much for digging into this thorny issue! The approach seems pretty good to me. Some things to tweak:
|
| @@ -103,7 +102,7 @@ def release_lock(lock): | |||
| logging.debug("released.") | |||
|
|
|||
|
|
|||
| def send_all(): | |||
| def send_all(): # noqa: C901 # TODO: refactor to reduce complexity | |||
There was a problem hiding this comment.
I'm personally not that worried about complexity here - I don't think clarity would be increased that much by refactoring it.
|
This looks good to me. |
| 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.
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.
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.
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.
|
To avoid confusion, I'm closing this PR, since I'm not going to be able to resolve the various issues it raises. It's linked in the original issue #73, and if someone wants to grab the code and resurrect it later that would be great. |
[NOT FOR MERGE yet: see "TODO" comments for discussion]
In engine.send_all:
and cleanly exiting send_all. (Previously, crashed
without logging.)
message. (Previously, behavior differed between
Python 2 and 3.)
RESULT_ERROR status, and deleting message from send
queue. (Previously, crashed without logging, leaving
possibly-unsendable message blocking send queue.)
Potentially-breaking change: If you are using a
MAILER_EMAIL_BACKEND other than the default
SMTP EmailBackend, errors from that backend may
be handled differently now.
Addresses #73. (And additional discussion there.)