Opened 9 years ago
Closed 8 years ago
#26210 closed Cleanup/optimization (fixed)
Make the SMTP more efficient if an error passes silently when creating a connection
Reported by: | Antonis Christofides | Owned by: | mlevental |
---|---|---|---|
Component: | Core (Mail) | Version: | 1.9 |
Severity: | Normal | Keywords: | |
Cc: | m.levental@…, Aleksej Manaev | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
My server had a 500 error, and Django was trying to email the error information to the ADMINS. Upon reaching https://github.com/django/django/blob/0ed7d15/django/core/mail/backends/smtp.py#L64, there was an exception because of a gevent bug.
In that case, fail_silently
is True. The result is that the open()
method returns as if everything had gone fine. This means that, subsequently, the send_messages()
method will be called, and so on.
Impact: Maybe this is just a bit ugly, however if you continue to work as if there had been no error when an error has actually occurred, you are asking for trouble. http://stackoverflow.com/questions/35315397/ shows why this issue made debugging another problem a day or two longer.
It should also be possible to log the error that occurs during the sending of the email, although this is probably a different issue.
Attachments (1)
Change History (8)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
It is intentional that the failure of the emailing be silent. However, the proper way of achieving this is the following pseudocode:
try: connect_to_the_mail_server(); say_ehlo(); start_tls(); say_ehlo(); login(); specify_mailfrom_rcptto_and_data(); close_connection_to_the_mail_server(); except: pass;
Instead, what it now does is this:
try: connect_to_the_mail_server(); say_ehlo(); start_tls(); say_ehlo(); login(); except: pass; try: specify_mailfrom_rcptto_and_data(); except: pass; try: close_connection_to_the_mail_server(); except: pass;
(The first block is what open()
does, the second is what send_messages()
does, and the third is what close()
does; these are methods in django.core.mail.backend.smtp.EmailBackend
.)
Call me a perfectionist (with deadlines), but it's really ugly to proceed with the second block if an error has occurred on the first block. I haven't seen any really bad consequences (other than that it made it harder for me to debug an unrelated problem I was having), but it's risky.
It's not hard to fix with a little restructuring of the code (unit testing such restructuring is the hardest part), but I'm not volunteering to do it, sorry :-)
comment:3 by , 9 years ago
Summary: | When emailing the admins about exceptions and an error occurs, it continues as if there had been no error → Make the SMTP more efficient if an error passes silently when creating a connection |
---|---|
Triage Stage: | Unreviewed → Accepted |
Type: | Bug → Cleanup/optimization |
I think the attached patch might do it. We'll just need a test.
by , 9 years ago
Attachment: | 26210.diff added |
---|
comment:4 by , 8 years ago
Cc: | added |
---|---|
Owner: | changed from | to
Status: | new → assigned |
comment:5 by , 8 years ago
Cc: | added |
---|
comment:6 by , 8 years ago
Has patch: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
PR looks good, pending a few cosmetic cleanups.
This looks like intentional behavior as described in #19637. I'm not sure what change we could make other than document it. Do you have a proposal?