Opened 11 years ago

Closed 11 years ago

#19214 closed New feature (wontfix)

AdminEmailHandler is synchronous.

Reported by: tdhutt@… Owned by: nobody
Component: Core (Mail) Version: 1.4
Severity: Normal Keywords: AdminEmailHandler
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Hi set up my logging settings to email me in certain situations (e.g. aliens visiting my site) by doing logger.warn("Visitor from space") in a middleware class, and then setting a logger for that middleware class to use AdminEmailHandler. However it is useless, because AdminEmailHandler is synchronous! Any aliens have to wait two or three seconds for the email to be sent before seeing my site!

It would be much much better if the email was asynchronous, or at least there was an additional AsyncAdminEmailHandler which was used by default.

Change History (6)

comment:1 by tdhutt@…, 11 years ago

Simple fix - use the following class instead of the built in AdminEmailHandler in your LOGGING thing.

import threading

# Class for running a function in another thread. TODO: Use this for the code in gcm and email2.
class FuncThread(threading.Thread):
    def __init__(self, target, *args, **kwargs):
        self._target = target
        self._args = args
        self._kwargs = kwargs
        threading.Thread.__init__(self)

    def run(self):
        self._target(*self._args, **self._kwargs)

import logging
import traceback

from django.conf import settings
from django.core import mail
from django.views.debug import ExceptionReporter, get_exception_reporter_filter


class AdminEmailHandler(logging.Handler):
    """An exception log handler that ASYNCHRONOUSLY emails log entries to site admins.

    If the request is passed as the first argument to the log record,
    request data will be provided in the email report.

    The default one is blocking, which is a little silly.
    """

    def __init__(self, include_html=False):
        logging.Handler.__init__(self)
        self.include_html = include_html

    def emit(self, record):
        try:
            request = record.request
            subject = '%s (%s IP): %s' % (
                record.levelname,
                (request.META.get('REMOTE_ADDR') in settings.INTERNAL_IPS
                 and 'internal' or 'EXTERNAL'),
                record.getMessage()
            )
            filter = get_exception_reporter_filter(request)
            request_repr = filter.get_request_repr(request)
        except Exception:
            subject = '%s: %s' % (
                record.levelname,
                record.getMessage()
            )
            request = None
            request_repr = "Request repr() unavailable."
        subject = self.format_subject(subject)

        if record.exc_info:
            exc_info = record.exc_info
            stack_trace = '\n'.join(traceback.format_exception(*record.exc_info))
        else:
            exc_info = (None, record.getMessage(), None)
            stack_trace = 'No stack trace available'

        message = "%s\n\n%s" % (stack_trace, request_repr)
        reporter = ExceptionReporter(request, is_email=True, *exc_info)
        html_message = self.include_html and reporter.get_traceback_html() or None
        FuncThread(mail.mail_admins, subject, message, fail_silently=True, html_message=html_message).start()

    def format_subject(self, subject):
        """
        Escape CR and LF characters, and limit length.
        RFC 2822's hard limit is 998 characters per line. So, minus "Subject: "
        the actual subject must be no longer than 989 characters.
        """
        formatted_subject = subject.replace('\n', '\\n').replace('\r', '\\r')
        return formatted_subject[:989]

comment:2 by Claude Paroz, 11 years ago

Component: Core (Other)Core (Mail)
Easy pickings: unset
Triage Stage: UnreviewedAccepted
Type: UncategorizedNew feature

There are Django applications dedicated to sending mail asynchronously (https://github.com/pinax/django-mailer/ or http://pypi.python.org/pypi/django-celery-email).

However I think it could be interesting to add an async flag to some Django mail API like mail_admins or mail_managers which would do the effective connection to the MTA in a thread (assuming fail_silently=True).

comment:3 by Łukasz Rekucki, 11 years ago

Making it async will only hide the fact that it's slow. Wouldn't it be better to keep the MTA connection open (or have a pool of open connections), as creating the connection is what most likely slows this down? I have a feeling this is a solution on a wrong level - isn't this something that is done in the email backend? And spawning a new thread on each call to emit() looks at least wasteful if not dangerous.

comment:4 by Aymeric Augustin, 11 years ago

I'm very wary of spawning threads in request handlers. It will work in trivial cases but I'm not sure application servers handle this very well under load.

lrekucki's proposal is interesting — and can be implemented as a custom email backend without any change in Django.

Last edited 11 years ago by Aymeric Augustin (previous) (diff)

comment:5 by Russell Keith-Magee, 11 years ago

Yeah... I'm a *HUGE* -1 on this.

Using threads like this is *monunmentally* bad idea. Consider - in the pathological case, your webserver will start up a clean environment for every request. If the thread is allowed to run disconnected, that means you'll have a thread running that you *hope* finishes. Alternatively, you'll need to join the thread before the request finishes, which means you'll end up with the same type of lockup behaviour (although, depending on how it's implemented, it might not be observable to the end-user -- it will just manifest as resource starvation due to an unavailable process on the web server.

In practice. your webserver won't be configured like this; but in practice, you'll also usually have a 'maximum requests served' limit on a process, so you'll never know if you're dealing with the "last" request that a particular process will be handling. You'll end up with the same problem, just harder to peg down.

We have an existing framework for fixing latency problems with sending mail -- it's the mail backend API. The default mail backend is dumb and synchronous. It's a reasonable and default that will work everywhere (once configured) and on low traffic sites will cause no problems. For higher traffic sites, there are plenty of implementations out there that are non-blocking. We don't fix *anything* by making one specific usage threaded -- or even the mail API itself -- threaded and/or asynchronous while still being bound to the request/response cycle.

I'm in favor of wontfixing this; or, at the very most, documenting the synchronous limitations of the default mail handler API.

comment:6 by Aymeric Augustin, 11 years ago

Resolution: wontfix
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top