-
Notifications
You must be signed in to change notification settings - Fork 336
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
Queue stalls when non-SMTP email backend raises error #73
Comments
Yes, this is a tricky issue. Another possibility is that we get Django to define and use a In fact, the current docs for fail_silently - https://docs.djangoproject.com/en/1.10/topics/email/#send-mail - are incorrect if you are not using the default backend. As a Django core dev, I would back this proposal. I'm unlikely to have the time to implement it. It could get tricky as well, because it would mean wrapping exceptions from the stdlib with Django's own exceptions - using something like this technique I guess http://stackoverflow.com/questions/3847503/wrapping-exceptions-in-python - but you would also need to dynamically subclass all exceptions, so that existing code catching I would be happy to add some interim solution to django-mailer as well (which would be the only solution if the Django change didn't work out). Probably the last of your suggestions is best, I can see the argument for it. If the Django changes go through, we could do
or something. |
Hmm... I suppose another option would be to directly use the smtplib exception classes as the generic EmailBackend exceptions: in the discussion above, But the more I dig into this, it seems like there are at least two distinct types of sending errors:
Deferring permanent errors isn't helpful, and can lead to other problems. (E.g., if you use EMAIL_MAX_BATCH or EMAIL_MAX_DEFERRED, you'd eventually get into a state where the number of deferred permanent errors would block retrying later-deferred transient errors.) So, I might expect send_all to end up something like: # (*many* details omitted)
for message in prioritize():
try:
connection.send(message.email)
except TransientSendError as err:
message.defer()
MessageLog.objects.log(message, RESULT_FAILURE, log_message=str(err))
except Exception as err: # anything else is "permanent error"
MessageLog.objects.log(message, RESULT_PERMANENT_FAILURE, log_message=str(err))
message.delete() # remove from queue -- *don't* try again later
else: # successful send
MessageLog.objects.log(message, RESULT_SUCCESS)
message.delete() (Though there's actually a third possible behavior in the existing django-mailer code: if the backend throws an unexpected exception, send_all immediately aborts the batch (crashes the send_all management command), without deferring the current message. If you're using the recommended cron scripts, it'll retry that send and continue the batch one minute later. And this turns out to be the ideal way to handle transient network issues with a transactional ESP backend like Anymail.) I think I'm leaning toward a combination of changing Anymail's exceptions to inherit from smtplib errors where meaningful, and then proposing a PR to django-mailer to split the individual smtplib exceptions between transient and permanent error handling as above. |
OK, it looks like you've thought this through, please go ahead and I'll look at your PR. I won't have time to work on it myself. |
If sending a message raises a "non-retryable" error, delete the Message from the queue, and note the error in the MessageLog with a new `RESULT_ERROR` status. (Retryable errors continue to be deferred and logged with `RESULT_FAILURE`.) This addresses two, related problems: 1. Deferring a message with an expected, but non-retryable error would over time grow the deferred queue (repeatedly hammering the backend with the same unsendable messages). 2. Sending a message with an unhandled error would interrupt the send_all, immediately and permanently stalling the entire queue. "Non-retryable" errors are: * Anything where the original message would need to be altered to be sent successfully. (E.g., a sender not recognized by your SMTP server, or a malformed recipient address.) Covers the first problem. * Catchall `Exception` for anything not explicitly enumerated as a transient, retryable error (like a network issue). The catchall covers most errors raised by third-party EmailBackends. Addresses the second problem. Addresses pinax#73.
OK, I've been struggling with how to fix the original problem, without also breaking some current, (probably-unintended but) valuable behavior. Apologies for the length; specific proposal and questions at the end. Here's how various sending exceptions are handled now (assuming recommended cron jobs and default settings):
Proposed changes
There would be no other changes to send_all exception handling. (In particular, I'm not going to try to distinguish retryable from non-retryable smtplib exceptions, which is non-trivial. They'll all just get deferred.) By default, third-party EmailBackend exceptions will fall under (B) and delete the unsent message. Backends could opt particular errors into deferred-retry (C) by subclassing |
[NOT FOR MERGE yet: see "TODO" for discussion] In engine.send_all: * Handle transient connection issues by logging error and cleanly exiting send_all. (Previously, crashed without logging.) * Handle all other smtplib.SMTPException by deferring message. (Previously, behavior differred between Python 2 and 3.) * Handle all other exceptions by logging error 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.)
[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.)
Facing the same issue when using sparkpost here, as this raises its own exceptions, which aren't caught and then the whole queue stalls and I have to manually delete the messages from the queue before it can continue..
I personally favor option B, maybe with the option to email the debug to the system administrators in that case? |
@medmunds Sorry for the really long delay on this one. These proposed changes sound good, assuming that as well as deleting from the queue, we create a MessageLog with the original data, so it is not lost forever. As I think you suggested we would use a new RESULT_CODE for this case. |
@spookylukey Sorry, I just realized this was still open and I'd been missing notifications on it. Unfortunately, I haven't been actively using django-mailer for some time now, and given that combined with the ambiguity of the various email exceptions, I'm not really confident I can move this forward without potentially injecting other problems. If someone else is able to adopt PR #78, that'd be fine, but I'd also completely understand if it makes more sense to just close it unmerged. |
Instead of trying to distinguish between transient and permanent errors, and possibly deleting emails (which is a deal breaker for my use cases), I propose a different approach: Count how many times a messages has been deferred (add a field to the model), and move messages deferred more than X times to a new state, in which they will no longer be retried, but will be kept in the DB for introspection. This requires a DB migration, but could simplify the code and be more robust. I would be willing to work on getting this implemented if there is interest! |
@taleinat I'm starting to feel that the only lib django-mailer should support out of the box is smtplib as it's the most logical. Anything else would require the developer to decide how to (better) handle their errors and more specifically errors from their chosen mailer backend. I believe that the PR I just submitted achieves that. Feedback very welcome. I've never submitted a PR before let alone some code that handles exceptions like this so I'm not even sure if this is acceptable best practice. |
@taleinat I think adding a retry count would still be beneficial. Adds that much more flexibility for the dev to decide how best to deal with error messages. Thinking that should be brought in through a separate PR. |
@volksman I'd be happy to help with that. However, if there isn't going to be specific built-in support for Amazon SES nor a plug-in mechanism enabling addition of such support, I won't be able to justify working on that at this point, since I won't actually be able to use this library on my current project. |
Hi! There are a bunch of current and past pull requests on this topic; my impression is this:
For ways to go forward I see:
For (b) and (c) I would volunteer, provided that someone has some time for review and permission to merge and wants to see this finished, too. What do you think? CC: @pakal @spookylukey |
I can't merge and I've finally integrated a different mailer approach, but feel free to ping me for some formal code reviews :) |
Okay thanks! @spookylukey what do you think? |
@hartwork . First very sorry for being unresponsive! I think the lack of progress on the more complex approaches means we should go for something simpler like your a) or c), probably c) if possible. This will be better than trying for a perfect solution. I will try to review a patch like that in a timely manner, and I can merge it. |
@hartwork @spookylukey I had to upgrade my project to django3 so I'm back with my suggestions in a PR again. Let me know... |
@volksman there are three open pull requests by you targetting #73 now. Pull requests can be re-used, updated, rebased, so ideally there would be one pull request that goes through a loop of review and fixing until some consensus and a solid implementation is reached. A quick look at #127 shows that method |
@hartwork It has been at least 2 years since my first PR. Rebasing seemed like a waste given the project has undergone an entire structural change. I've closed off the old PR in favour of this one and the original bandaid that I still use in production. Sorry I don't know all the ins and outs of Github, CI etc...I work alone for the most part so collaboration is not my strong suit, however if it's really that bothersome I can stop trying to contribute. Would just be nice to have a solution one way or another with regard to transactional email providers vs traditional SMTP. I can live off my fork as I have for the last 2 years. All the best! |
#91 can be closed off as it will never work anymore, however the solution is still sound and works great for my needs. That is my current implementation in production on a couple sites. |
I see. @volksman could you rebase #91 in place to fix its conflicts with |
@volksman @hartwork
|
Not able to rebase as I destroyed my fork and GitHub doesn't allow you to replace it in the PR. I have added a small paragraph explaining the optional settings key and the optional override of the error handler. I don't believe there are any backwards compatibility issues. I am now using the error_handler (#127 ) branch on one of my production servers without any changes (not overriding the error handler). |
@spookylukey sounds like a plan — thank you! Can you close #91 and #85 with a hint on #127 maybe, for us? |
Fixed via #133 |
I'm using django-mailer with a django-anymail backend (specifically,
MAILER_EMAIL_BACKEND = "anymail.backends.postmark.PostmarkBackend"
).Anymail is not based on smtplib, so it raises its own exceptions for rejected recipients or other errors. Because django-mailer is looking for specific SMTP exceptions, an Anymail exception ends up stalling the queue, rather than deferring the unsendable message. (The exception aborts the send_mail management command, leaving the unsendable message at the top of the prioritization list for the next send.)
What would be a reasonable way to solve this? (I maintain django-anymail, btw.)
SendMailException
(e.g.), in addition to the current SMTP exceptions. Then other packages that want to play nice with django-mailer could raisemailer.SendMailException
when django-mailer is installed. (I'm not at all opposed to adding this to Anymail. But it would sort of start the opposite slippery slope, where email backends need to add specific code for any likely queueing wrappers.)Exception
, rather than specific SMTP exceptions. I realize broad catch statements are generally a bad idea, but this seems like a case where it might be appropriate.Thoughts? I'm happy to submit a PR if one of the django-mailer changes makes sense.
The text was updated successfully, but these errors were encountered: