Opened 8 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)

26210.diff (1.1 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Tim Graham, 8 years ago

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?

comment:2 by Antonis Christofides, 8 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 Tim Graham, 8 years ago

Summary: When emailing the admins about exceptions and an error occurs, it continues as if there had been no errorMake the SMTP more efficient if an error passes silently when creating a connection
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I think the attached patch might do it. We'll just need a test.

by Tim Graham, 8 years ago

Attachment: 26210.diff added

comment:4 by mlevental, 8 years ago

Cc: m.levental@… added
Owner: changed from nobody to mlevental
Status: newassigned

comment:5 by Aleksej Manaev, 8 years ago

Cc: Aleksej Manaev added

comment:6 by Tim Graham, 8 years ago

Has patch: set
Triage Stage: AcceptedReady for checkin

PR looks good, pending a few cosmetic cleanups.

comment:7 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: assignedclosed

In 42dc9d04:

Fixed #26210 -- Prevented SMTP backend from trying to send mail after a connection failure.

Note: See TracTickets for help on using tickets.
Back to Top