The built-in behavior for
django.core.handlers.base.handle_uncaught_exception calls mail_admins
for each internal server error.
So if a very high-traffic view has an internal server error, duplicate
emails will be sent at a very high rate. This isn't usually
desirable...
We worked around this by changing mail_admins directly in our copy of
Django to store the last time an email was sent on the filesystem and
avoid sending multiple emails in too short of a time interval. But it
seems like there should be a better way to customize this without
having to modify Django.
I suppose one could define a mail_admins replacement in our app and
assign it to django.core.mail.mail_admins. But that's rather hacky...
Also, BaseHandler says that handle_uncaught_exception can be
overridden, but its derived classes ModPythonHandler and WSGIHandler
are referenced directly in Django, so I don't really see how to
subclass it without changing the Django code.
I was thinking it would be useful to add a setting like
EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default
would look something like this:
> I was thinking it would be useful to add a setting like > EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default > would look something like this:
I like the idea, but not the addition of yet another setting. Maybe an on_exception signal could be added which can do its own thing and return False if the default action shouldn't be taken?
I see there is a got_request_exception signal already... so one could
effectively do the same thing by adding a signal handler and making
settings.ADMINS the empty list so that mail_admins effectively becomes
a no-op.
Even so, it looks like mail_admins will open a SMTP connection to send
an email message even if the message has no recipients. So it seems
wrong to force every non-debug exception to go through that code path.
(If your servers are melting down, opening hundreds of SMTP
connections probably won't help the situation.)
-Jesse
On Oct 15, 2:18 pm, "Jeremy Dunck" <jdu...@gmail.com> wrote:
> On Wed, Oct 15, 2008 at 3:58 PM, Jesse Young <adu...@gmail.com> wrote:
> ...
> > I was thinking it would be useful to add a setting like
> > EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default
> > would look something like this:
> I like the idea, but not the addition of yet another setting. Maybe
> an on_exception signal could be added which can do its own thing and
> return False if the default action shouldn't be taken?
On Wed, Oct 15, 2008 at 6:26 PM, Jesse Young <adu...@gmail.com> wrote:
> I see there is a got_request_exception signal already... so one could > effectively do the same thing by adding a signal handler and making > settings.ADMINS the empty list so that mail_admins effectively becomes > a no-op.
> Even so, it looks like mail_admins will open a SMTP connection to send > an email message even if the message has no recipients. So it seems > wrong to force every non-debug exception to go through that code path. > (If your servers are melting down, opening hundreds of SMTP > connections probably won't help the situation.)
In handlers.base.get_response, theres: " receivers = dispatcher.send(signal=signals.got_request_exception, request=request) "
Return value cases: In [2]: l = [] #no receivers In [3]: l2 = [None] #only old receivers In [4]: l3 = [False] #new receiver asking not to default handle In [5]: l4 = [None, False] #old and new receiver In [6]: l5 = [True, False] #mix of new receivers, one asking not to default handle
The odd edge case is where a different signal handlers return both True and False (where existing code would return None). What to do there is a design choice; I'd vote for False wins.
So I'd add: " if receivers: default_handler = not bool([ret_value for ret_value in receivers if ret_value == False]) else: default_handler = True
On Wed, 2008-10-15 at 13:58 -0700, Jesse Young wrote: > The built-in behavior for > django.core.handlers.base.handle_uncaught_exception calls mail_admins > for each internal server error.
> So if a very high-traffic view has an internal server error, duplicate > emails will be sent at a very high rate. This isn't usually > desirable...
So the reason handle_uncaught_exception is in its own method is, as noted in the docstring for that method, so that you can have exactly this type of control. I have one client, for example, who want to use Python's logging module instead of sending email and no doubt lots of other customisations are possible.
The idea is that you create your own subclass of which django/core/handlers/* class you want (wsgi or modpython, typically) and override the handle_uncaught_exception method. After all, you specify the handler class to use when setting things up to talk to the webserver, so it's just as easy to use your own subclass as one of Django's built in versions.
This is done via subclassing rather than a setting to avoid settings proliferation for something where there's no real common case of customisations and so the ultimate configuration is provided: you write Python code to do whatever you want. It's also meant to impose a slight barrier to entry in the sense that you have to be able to write Python code in order to customise this, which is reasonable for something as core-functionality oriented as handling uncaught exceptions (and no problems for people on django-dev, obviously).
Thanks for explaining the rationale behind this. It'd certainly be
possible to do what we want by overriding handle_uncaught_exception in
a ModPythonHandler subclass.
This method has a higher barrier to entry than I'd like, though. For
one, this kind of customization requires updating the apache
configuration file to point to the new handler. It's annoying to have
to update Apache config files in synchronization with the Python app
code.
Also, I suspect that a large number of Django developers use
"manage.py runserver" (which is hardcoded to use WSGIHandler) for
development and testing and use ModPythonHandler on the release
servers. To minimize unexpected bugs in the release environment that
never show up in the test environment, it's best not to introduce
additional differences between these environments. A custom
implementation of handle_uncaught_exception would be hard for a
developer using "manage.py runserver" to test.
My hunch is that even though there isn't a common way that users would
want to customize uncaught exceptions, it's probably common that users
would want to customize it in some way or another. So subclassing the
handler seems like a rather high barrier to entry.
-Jesse
On Oct 15, 6:15 pm, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
> On Wed, 2008-10-15 at 13:58 -0700, Jesse Young wrote:
> > The built-in behavior for
> > django.core.handlers.base.handle_uncaught_exception calls mail_admins
> > for each internal server error.
> > So if a very high-traffic view has an internal server error, duplicate
> > emails will be sent at a very high rate. This isn't usually
> > desirable...
> So the reason handle_uncaught_exception is in its own method is, as
> noted in the docstring for that method, so that you can have exactly
> this type of control. I have one client, for example, who want to use
> Python's logging module instead of sending email and no doubt lots of
> other customisations are possible.
> The idea is that you create your own subclass of which
> django/core/handlers/* class you want (wsgi or modpython, typically) and
> override the handle_uncaught_exception method. After all, you specify
> the handler class to use when setting things up to talk to the
> webserver, so it's just as easy to use your own subclass as one of
> Django's built in versions.
> This is done via subclassing rather than a setting to avoid settings
> proliferation for something where there's no real common case of
> customisations and so the ultimate configuration is provided: you write
> Python code to do whatever you want. It's also meant to impose a slight
> barrier to entry in the sense that you have to be able to write Python
> code in order to customise this, which is reasonable for something as
> core-functionality oriented as handling uncaught exceptions (and no
> problems for people on django-dev, obviously).
> The built-in behavior for
> django.core.handlers.base.handle_uncaught_exception calls mail_admins
> for each internal server error.
> So if a very high-traffic view has an internal server error, duplicate
> emails will be sent at a very high rate. This isn't usually
> desirable...
> We worked around this by changing mail_admins directly in our copy of
> Django to store the last time an email was sent on the filesystem and
> avoid sending multiple emails in too short of a time interval. But it
> seems like there should be a better way to customize this without
> having to modify Django.
> I suppose one could define a mail_admins replacement in our app and
> assign it to django.core.mail.mail_admins. But that's rather hacky...
> Also, BaseHandler says that handle_uncaught_exception can be
> overridden, but its derived classes ModPythonHandler and WSGIHandler
> are referenced directly in Django, so I don't really see how to
> subclass it without changing the Django code.
> I was thinking it would be useful to add a setting like
> EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default
> would look something like this:
Is your suggestion that, since mail_admins happens to be the only
place in Django that uses settings.ADMINS, I could do something like:
class AdminsObject(list):
def __iter__(self):
// do some custom notification
// manually write the friendly 500 error page to the output
stream somehow
raise Exception // can't raise StopIteration here or mail_admins
will connect to the SMTP server
ADMINS = AdminsObject()
That would be pretty hilarious. Not particularly straightforward or
maintainable though.
-Jesse
On Oct 15, 9:35 pm, oggie rob <oz.robhar...@gmail.com> wrote:
> Did you try subclassing list (& overriding __iter__) for the ADMINS
> object?
> -rob
> On Oct 15, 1:58 pm, Jesse Young <adu...@gmail.com> wrote:
> > The built-in behavior for
> > django.core.handlers.base.handle_uncaught_exception calls mail_admins
> > for each internal server error.
> > So if a very high-traffic view has an internal server error, duplicate
> > emails will be sent at a very high rate. This isn't usually
> > desirable...
> > We worked around this by changing mail_admins directly in our copy of
> > Django to store the last time an email was sent on the filesystem and
> > avoid sending multiple emails in too short of a time interval. But it
> > seems like there should be a better way to customize this without
> > having to modify Django.
> > I suppose one could define a mail_admins replacement in our app and
> > assign it to django.core.mail.mail_admins. But that's rather hacky...
> > Also, BaseHandler says that handle_uncaught_exception can be
> > overridden, but its derived classes ModPythonHandler and WSGIHandler
> > are referenced directly in Django, so I don't really see how to
> > subclass it without changing the Django code.
> > I was thinking it would be useful to add a setting like
> > EXCEPTION_NOTIFIER = 'path.to.custom.notifier' , where the default
> > would look something like this:
Jesse Young wrote: > The built-in behavior for > django.core.handlers.base.handle_uncaught_exception calls mail_admins > for each internal server error.
> So if a very high-traffic view has an internal server error, duplicate > emails will be sent at a very high rate. This isn't usually > desirable...
We're working it around by not having settings.ADMINS set in production environment. Instead we log unhandled exceptions by listening to `signals.got_request_exception`. I don't even consider it as a workaround actually but as a normal solution :-)
On Oct 15, 10:18 pm, Jesse Young <adu...@gmail.com> wrote:
> Is your suggestion that, since mail_admins happens to be the only
> place in Django that uses settings.ADMINS, I could do something like:
> class AdminsObject(list):
> def __iter__(self):
> // do some custom notification
> // manually write the friendly 500 error page to the output
> stream somehow
> raise Exception // can't raise StopIteration here or mail_admins
> will connect to the SMTP server
> ADMINS = AdminsObject()
I was hoping it wouldn't be quite that ugly! And just had an empty
list.iter in mind. Something like:
class AdminsObject(list):
def __iter__(self):
if ok_to_send(): # enough time since last mail or whatever
return super(AdminsObject, self).__iter__()
return [].__iter__()
I think your exception would propagate up to the
handle_uncaught_exception and the user would not see the "friendly
error message".
However, after seeing your comment regarding the exception I looked at
EmailMessage and was surprised to see that it attempts to send even
with an empty ADMINS list. It should return if not self.to and not
self.bcc, I think.
> That would be pretty hilarious. Not particularly straightforward or
> maintainable though.
Yes, the got_request_exception signal seems like a better solution as
it gives you more context.
On Wed, 2008-10-15 at 18:59 -0700, Jesse Young wrote: > Hi Malcolm,
> Thanks for explaining the rationale behind this. It'd certainly be > possible to do what we want by overriding handle_uncaught_exception in > a ModPythonHandler subclass.
> This method has a higher barrier to entry than I'd like, though. For > one, this kind of customization requires updating the apache > configuration file to point to the new handler. It's annoying to have > to update Apache config files in synchronization with the Python app > code.
Well, not really. You have to set the Apache configuration once to point to the right thing. Just like you do now.
> Also, I suspect that a large number of Django developers use > "manage.py runserver" (which is hardcoded to use WSGIHandler) for > development and testing and use ModPythonHandler on the release > servers. To minimize unexpected bugs in the release environment that > never show up in the test environment, it's best not to introduce > additional differences between these environments. A custom > implementation of handle_uncaught_exception would be hard for a > developer using "manage.py runserver" to test.
Patches welcome to fix that if you see it as a real problem.
> My hunch is that even though there isn't a common way that users would > want to customize uncaught exceptions, it's probably common that users > would want to customize it in some way or another. So subclassing the > handler seems like a rather high barrier to entry.
No it's not. Since there's no common way to customize, sub-classing is the perfect way to do it, since people can then do whatever they want. How is asking somebody to write a subclass in Python a high barrier to entry? We're assuming knowledge of the language, but that's all.
I don't see any of your objections here a real issues except the need for being able to simulate things in the development server.
> No it's not. Since there's no common way to customize, sub-classing is
> the perfect way to do it, since people can then do whatever they want.
> How is asking somebody to write a subclass in Python a high barrier to
> entry? We're assuming knowledge of the language, but that's all.
Well, I'm not saying that writing a subclass in Python is a high
barrier to entry. I'm saying that the handler in particular is hard to
subclass. And I agree that subclassing it would be 'the perfect way',
if only it didn't require a configuration change (yes, only once, but
that is once more than required by basically any other change to a
Django app) and didn't introduce differences between the test and
release environments.
I mean, really, the notification method for errors should have nothing
to do with the type of web server interface you have (WSGI /
mod_python). It's just an artifact of the way the code is currently
factored.
> Patches welcome to fix that if you see it as a real problem.
I suppose the easiest way is to patch mail_admins to not attempt to
send email if ADMINS is empty.
On Oct 16, 2:41 pm, Malcolm Tredinnick <malc...@pointy-stick.com>
wrote:
> On Wed, 2008-10-15 at 18:59 -0700, Jesse Young wrote:
> > Hi Malcolm,
> > Thanks for explaining the rationale behind this. It'd certainly be
> > possible to do what we want by overriding handle_uncaught_exception in
> > a ModPythonHandler subclass.
> > This method has a higher barrier to entry than I'd like, though. For
> > one, this kind of customization requires updating the apache
> > configuration file to point to the new handler. It's annoying to have
> > to update Apache config files in synchronization with the Python app
> > code.
> Well, not really. You have to set the Apache configuration once to point
> to the right thing. Just like you do now.
> > Also, I suspect that a large number of Django developers use
> > "manage.py runserver" (which is hardcoded to use WSGIHandler) for
> > development and testing and use ModPythonHandler on the release
> > servers. To minimize unexpected bugs in the release environment that
> > never show up in the test environment, it's best not to introduce
> > additional differences between these environments. A custom
> > implementation of handle_uncaught_exception would be hard for a
> > developer using "manage.py runserver" to test.
> Patches welcome to fix that if you see it as a real problem.
> > My hunch is that even though there isn't a common way that users would
> > want to customize uncaught exceptions, it's probably common that users
> > would want to customize it in some way or another. So subclassing the
> > handler seems like a rather high barrier to entry.
> I don't see any of your objections here a real issues except the need
> for being able to simulate things in the development server.
On Thu, 2008-10-16 at 15:15 -0700, Jesse Young wrote: > > No it's not. Since there's no common way to customize, sub-classing is > > the perfect way to do it, since people can then do whatever they want. > > How is asking somebody to write a subclass in Python a high barrier to > > entry? We're assuming knowledge of the language, but that's all.
> Well, I'm not saying that writing a subclass in Python is a high > barrier to entry. I'm saying that the handler in particular is hard to > subclass. And I agree that subclassing it would be 'the perfect way', > if only it didn't require a configuration change (yes, only once, but > that is once more than required by basically any other change to a > Django app)
No, this isn't correct. You already have to specify which handler to run. You still have to do that exactly once in each case.
> and didn't introduce differences between the test and > release environments.
There's already differences between the test and release environments if you mean runserver versus anything running with a real webserver. Expecting complete parity there is a false goal and you absolutely must do real production-preparedness tests against the real webserver you're going to use in production. That's hardly a surprise. A patch to allow something like --with-handler='my.handler.class' (making up an API on the spot; that needs to be thought about) would be a good idea for completeness, though, you're right.
> I mean, really, the notification method for errors should have nothing > to do with the type of web server interface you have (WSGI / > mod_python).
The method is called handle_uncaught_exception, not notify_about_uncaught_exception. Right now it only happens to do notification, but because it's extensible it could do stuff that is handler dependent.
The --with-handler idea for manage.py runserver sounds reasonable to
me.
I just wanted to clarify a couple of points.
> No, this isn't correct. You already have to specify which handler to
> run. You still have to do that exactly once in each case.
I meant that it requires one extra configuration change if you've
already deployed your app using the default handler, and later want to
override handle_uncaught_exception.
> There's already differences between the test and release environments if
> you mean runserver versus anything running with a real webserver.
> Expecting complete parity there is a false goal
Sure, but that doesn't mean I shouldn't try to avoid making the test/
release environments more different than they already are.
|
