Opened 13 years ago
Closed 13 years ago
#19214 closed New feature (wontfix)
AdminEmailHandler is synchronous.
| Reported by: | 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 , 13 years ago
comment:2 by , 13 years ago
| Component: | Core (Other) → Core (Mail) |
|---|---|
| Easy pickings: | unset |
| Triage Stage: | Unreviewed → Accepted |
| Type: | Uncategorized → New 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 , 13 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 , 13 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.
comment:5 by , 13 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 , 13 years ago
| Resolution: | → wontfix |
|---|---|
| Status: | new → closed |
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]