Opened 6 weeks ago

Closed 4 days ago

Last modified 4 days ago

#37261 closed Cleanup/optimization (fixed)

Document specifics for AdminEmailHandler and BrokenLinkEmailsMiddleware migration to MAILERS

Reported by: Adam Johnson Owned by: Mike Edmunds
Component: Core (Mail) Version: 6.1
Severity: Normal Keywords:
Cc: 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

Since #35514, Django’s built-in error reporting emails (admin emails and broken-link emails) raise on any transport error when the new dictionary-based MAILERS setting is configured, instead of failing silently as they did in Django 6.0.

Specifically:

  • AdminEmailHandler.emit() has no surrounding try/except, and Python's logging machinery does not contain exceptions raised by emit(), so the transport error is raised at the logger.error(...)/logger.exception(...) call site — i.e. inside Django's own error-handling path while reporting a 500.
  • BrokenLinkEmailsMiddleware.process_response() raises on every referred 404, turning it into a 500.

The new "Migrating email to mailers" howto recommends wrapping sends in an error handler with try: / except Exception: pass "to avoid cascading failures inan error handler that sends mail".
We need to apply that advice to Django's own built-in error handlers!

Change History (17)

comment:1 by Mike Edmunds, 6 weeks ago

Resolution: invalid
Status: assignedclosed

This is as designed. See https://github.com/django/deps/blob/main/accepted/0018-mailers.md#fail_silently-compatibility.

There's more discussion of the rationale behind this and several alternatives we considered in the forum: https://forum.djangoproject.com/t/deprecating-or-changing-how-django-core-mail-handles-fail-silently/44278/19 (and the lengthy series of preceding comments).

in reply to:  1 comment:2 by Adam Johnson, 6 weeks ago

Replying to Mike Edmunds:

This is as designed. See https://github.com/django/deps/blob/main/accepted/0018-mailers.md#fail_silently-compatibility.

There, I see:

Django's two internal uses of fail_silently=True will be replaced with MailerDoesNotExist checks. (See AdminEmailHandler and BrokenLinkEmailsMiddleware earlier.)

But I don't see rationale about why not try: ... except Exception: ... instead.

Replying to Mike Edmunds:

There's more discussion of the rationale behind this and several alternatives we considered in the forum: https://forum.djangoproject.com/t/deprecating-or-changing-how-django-core-mail-handles-fail-silently/44278/19 (and the lengthy series of preceding comments).

I read the thread, and it seems to cover fail_silently in general but not these two specific use cases.

---

I understand that removing fail_silently was completely agreed-on, but I think dropping its "continue on failure" behaviour in these two specific cases was overreach. To me, AdminEmailHandler and BrokenLinkEmailsMiddleware only send emails as a best-effort. The failure to send an email should NOT crash the process/request:

  • AdminEmailHandler: A process that tries to log to emails admins should continue regardless of whether the email got through. Other logging handlers are peppered with try: ... except Exception: pass. *All* of Python’s built-in handler classes do this - see the emit() methods in the logging/handler.py source, including `SMTPHandler.emit()`
  • BrokenLinkEmailsMiddleware: A broken link should 404 regardless of whether the email could be sent. Crashing and returning an error page is unacceptable degradation when an email service provider is down, and could cause search indexers to start treating a given page differently.

comment:3 by Adam Johnson, 6 weeks ago

Resolution: invalid
Status: closednew

Right, I spotted in the fail_silently thread, post 8, you replied to Jacob:

Jacob:

I’m writing a reusable app: I’d like to send some mail, so long as my users have email configured at all. [emphasis added]

:light_bulb:Ah-hah! That’s what I had been missing, and totally makes sense to me. It would explain why Django uses fail_silently=True in AdminLogHandler and BrokenLinksMiddleware, as well as several examples I saw in the wild.

It seems the only rationale for the fail_silently usage in these two features that was discussed in the thread was this one. It doesn't cover my one: "these features shouldn't break things when email providers are down".

I think therefore we need input from others.

comment:4 by Mike Edmunds, 6 weeks ago

The exact earlier behavior is available with MAILERS by configuring the default mailer with "OPTIONS": {"fail_silently": True}`. fwiw, it ignores some (but not all) SMTP configuration errors, and some (but not all) send-time errors. That SMTP backend configuration option is not deprecated (although it is discouraged in the docs).

There was general agreement that AdminEmailHandler and BrokenLinkEmailsMiddleware should not silently swallow configuration errors that prevented sending the error emails altogether, and it was a historic accident that they did in earlier releases. A similar argument applied to unexpected SMTP transmission failures. We didn't identify a use case for expected SMTP failures that should be ignored. It sounds like you may have one, but I'm not certain it generalizes to all email backends. Ignoring all exceptions is considerably more broad and (imho) problematic because it hides configuration problems.

Agreed, more discussion is needed.

comment:5 by Adam Johnson, 5 weeks ago

It totally makes sense to me to raise configuration errors in most use cases, but not where the emails are best-effort debugging aids, like in the two features here.

in reply to:  5 comment:6 by Natalia Bidart, 5 weeks ago

Resolution: needsinfo
Status: newclosed

Replying to Adam Johnson:

It totally makes sense to me to raise configuration errors in most use cases, but not where the emails are best-effort debugging aids, like in the two features here.

Hello! A few thoughts: I don't share the "best-effort debugging aid" framing. ADMINS and MANAGERS are both empty by default and, sendtestemail aside, their only effect is to turn on exactly these emails. A project that sets them has made a deliberate choice to be notified. For anything genuinely best-effort you would reach for logging, error tracking, or telemetry that already routes into an alerting system. So a mailer that has silently stopped delivering is a real operational problem, and Mike's concern about hiding it is one I share: except Exception: pass means the notifications you asked for stop arriving and nothing tells you. We need loud failures in this case.

Before considering next steps, is there a report of this actually affecting a production project? If someone has run into this with a real provider outage or a misconfigured mailer in 6.1, that would be helpful to know and understand, to settle both the severity and the question of which exceptions matter in practice. Mike's point in comment:4 that we never identified a concrete case for ignoring expected SMTP failures is still kind of unanswered.

comment:7 by Mike Edmunds, 5 weeks ago

There's perhaps an argument that this should be covered explicitly in the deprecation notes and migration docs. Something along the lines of:

  • If a project is using AdminEmailHandler and/or BrokenLinkEmailsMiddleware
  • and it depends on the (undocumented, but pre-existing) behavior that those silently ignore transmission errors to ADMINS/MANAGERS (or that they ignore certain SMTP configuration errors)
  • then when the project opts into MAILERS, it will need to define a custom MAILERS config with "fail_silently": True in the OPTIONS and use that config for AdminEmailHandler's using argument or a BrokenLinkEmailsMiddleware subclass that overrides the using attribute.

[I'd also be tempted to suggest those projects could benefit from switching to infrastructure that queues and retries intermittent email failures, rather than relying on ignoring them. Particularly if they send any other email at all.]

I kind of think documentation is the best we can do for this. I can't really come up with a reliable, useful way to issue deprecation warnings for "you seem to be relying on AdminEmailHandler or BrokenLinkEmailsMiddleware ignoring transmission errors, which will change in Django 7.0."

in reply to:  7 comment:8 by Natalia Bidart, 12 days ago

Has patch: unset
Resolution: needsinfo
Severity: Release blockerNormal
Status: closednew
Summary: AdminEmailHandler and BrokenLinkEmailsMiddleware raise on email transport errors when MAILERS is configuredDocument specifics for AdminEmailHandler and BrokenLinkEmailsMiddleware migration to MAILERS
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Replying to Mike Edmunds:

There's perhaps an argument that this should be covered explicitly in the deprecation notes and migration docs.

I'm happy to reopen and repurpose this ticket for the doc extension.

comment:9 by Natalia Bidart, 12 days ago

Owner: Adam Johnson removed
Status: newassigned

comment:11 by Natalia Bidart, 12 days ago

Status: assignednew

comment:12 by Mike Edmunds, 12 days ago

Owner: set to Mike Edmunds
Status: newassigned

comment:14 by Natalia Bidart, 4 days ago

Patch needs improvement: set

comment:15 by Mike Edmunds, 4 days ago

Patch needs improvement: unset

comment:16 by Natalia Bidart, 4 days ago

Triage Stage: AcceptedReady for checkin

comment:17 by nessita <124304+nessita@…>, 4 days ago

Resolution: fixed
Status: assignedclosed

In bbf5a99:

Fixed #37261 -- Documented error reporting migration to MAILERS.

Documented specifics for migrating AdminEmailHandler and
BrokenLinkEmailsMiddleware to MAILERS when relying on fail_silently with
an unreliable email backend connection.

comment:18 by Natalia <124304+nessita@…>, 4 days ago

In 3107ba9:

[6.1.x] Fixed #37261 -- Documented error reporting migration to MAILERS.

Documented specifics for migrating AdminEmailHandler and
BrokenLinkEmailsMiddleware to MAILERS when relying on fail_silently with
an unreliable email backend connection.

Backport of bbf5a998c02b52f1b72474e4b310cff94f24ff36 from main.

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