[Django] #26210: When emailing the admins about exceptions and an error occurs, it continues as if there had been no error
Django
as if there had been no error
-----------------------------+--------------------
Reporter: aptiko | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: 1.9
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-----------------------------+--------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/26210>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
as if there had been no error
Reporter: aptiko | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:1>
Django
as if there had been no error
Reporter: aptiko | Owner: nobody
Type: Bug | Status: new
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aptiko):
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 :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:2>
Django
connection
--------------------------------------+------------------------------------
Reporter: aptiko | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Bug => Cleanup/optimization
* stage: Unreviewed => Accepted
Comment:
I think the attached patch might do it. We'll just need a test.
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:3>
Django
connection
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* Attachment "26210.diff" added.
Django
connection
Reporter: aptiko | Owner: mlevental
Type: | Status: assigned
Cleanup/optimization |
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by mlevental):
* status: new => assigned
* owner: nobody => mlevental
* cc: m.levental@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:4>
Django
connection
-------------------------------------+-------------------------------------
Reporter: aptiko | Owner: mlevental
Type: | Status: assigned
Cleanup/optimization |
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by AleksejManaev):
* cc: AleksejManaev (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:5>
Django
connection
-------------------------------------+-------------------------------------
Christofides |
Type: | Status: assigned
Cleanup/optimization |
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
* stage: Accepted => Ready for checkin
Comment:
[https://github.com/django/django/pull/7271 PR] looks good, pending a few
cosmetic cleanups.
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:6>
Django
connection
-------------------------------------+-------------------------------------
Reporter: Antonis | Owner: mlevental
Christofides |
Cleanup/optimization |
Component: Core (Mail) | Version: 1.9
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"42dc9d04006c0bdecba6e0c8a29038b01d5a62d7" 42dc9d04]:
{{{
#!CommitTicketReference repository=""
revision="42dc9d04006c0bdecba6e0c8a29038b01d5a62d7"
Fixed #26210 -- Prevented SMTP backend from trying to send mail after a
connection failure.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/26210#comment:7>