[Django] #21777: Make request exception handling more robust to subsequent exceptions
Django
-------------------------------+--------------------
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
In django/core/handlers/base.py, there is the following piece of code:
except: # Handle everything else.
# Get the exception info now, in case another exception is
thrown later.
signals.got_request_exception.send(sender=self.__class__,
request=request)
response = self.handle_uncaught_exception(request, resolver,
sys.exc_info())
Contrary to the comment, the exception info is saved too late. If, during
the got_request_exception signal handling another exception occurs (and
gets ignored), it will overwrite sys.exc.info(), and this subsequent
exception will get logged, instead of the original one.
I hit this while playing with MySQL's SSCursor (for exporting a lot of
data to CSV). Some exception happened in my application, then Django's
default got_request_exception handler kicked in and attempted to rollback.
This failed with a ProgrammingError, because not all rows were consumed,
and this ProgrammingError went into the traceback e-mail instead of the
original exception.
--
Ticket URL: <https://code.djangoproject.com/ticket/21777>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
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 anonymous):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
My diagnosis was incomplete. It is not only that exc_info is saved too
late. It is also needed to put a try ... except around the
signals.got_request_exception.send(...) so that
handle_uncaught_exception() is actually called.
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:1>
Django
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by numerodix):
* cc: numerodix@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:2>
Django
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anubhav9042):
It would be really nice if you could add the code where you came around
with this problem....
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:3>
Django
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by anonymous):
Yes, I understand that it would be a good idea to add a reproducer.
However, the original code that led me to this report is under NDA, so I
have to make another reproducer. Unfortunately, I will be able to do that
only after March, 14th. Sorry for that.
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:4>
Django
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by patrakov@…):
OK, here is the simplest possible example code.
bug21777/urls.py:
{{{
from django.conf.urls import patterns, include, url
urlpatterns = patterns('',
url(r'^$', 'bug21777.views.buggyview'),
)
}}}
Relevant part of bug21777/settings.py, enough to reproduce the bug:
{{{
SECRET_KEY = 'dummy'
DEBUG = False
TEMPLATE_DEBUG = False
ALLOWED_HOSTS = ['*']
ADMINS = (('admin', 'ad...@example.com'),)
EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'
INSTALLED_APPS = (
'bug21777',
)
MIDDLEWARE_CLASSES = (
)
ROOT_URLCONF = 'bug21777.urls'
WSGI_APPLICATION = 'bug21777.wsgi.application'
}}}
bug21777/views.py:
{{{
from django.core.signals import got_request_exception
def buggyview(request):
raise ValueError("Let's pretend this is a bug in my application")
def handler(signal, sender, **kwargs):
# The handler is buggy, too
raise KeyError("Let's pretend that the handler itself is confused by
the original exception")
got_request_exception.connect(handler)
}}}
Test with ./manage.py runserver
Note: here it is important to understand the assumptions not expressed in
the code above. In my case, the handler was in some sense confused by the
original exception, not just "buggy by itself". So the original exception
in the view represents the primary problem, and the exception in the
handler is just a nasty consequence.
If you modify the handler so that it is non-buggy, and load the site root
URL, then you'll notice that Django sends the admin a nice e-mail with a
traceback pointing to the actual buggy view function, and with ValueError.
This is good. I have not put it into the above example, but in production
we also log all such errors to Apache logs, using logging.StreamHandler.
With the buggy handler, as written above, the e-mail is not sent, no debug
output mentions the original ValueError anywhere, and the KeyError related
to the confused handler gets logged to stderr. This makes it very hard to
debug the original problem in the view. In my opinion, the ideal situation
would be if both exceptions were properly logged. Especially since the
comment mentions the possibility of "another exception" and expresses the
intention to avoid overwriting the original exception info.
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:5>
Django
Reporter: patrakov@… | Owner: nobody
Type: Uncategorized | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by anonymous):
* stage: Unreviewed => Accepted
Comment:
Yup, there's totally a codepath where an exception can escape
`get_response`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:6>
Django
Reporter: patrakov@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timo):
* type: Uncategorized => Cleanup/optimization
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:7>
Django
Reporter: patrakov@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: 1.6
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by blueyed):
* cc: django@… (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:8>
Django
Reporter: patrakov@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* component: Core (Other) => HTTP handling
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:9>
Django
Reporter: patrakov@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: 1.6
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Ludwig Kjellström):
Hi, I pulled a random ticket and landed here.
This is still not handled very well. (Even though PEP 3134 Exception
Chaining https://www.python.org/dev/peps/pep-3134/ has led to a good error
message once it gets printed.).
With `DEBUG=True` an error in the error handler leads to plain text `A
server error occurred. Please contact the administrator.`.
Current reproducible project is found here, based upon Patrakovs code:
https://github.com/metteludwig/django-bug-21777/tree/master
A trivial solution is to wrap the signals-call in try/catch and then
trivially pass along ''the new'' exception to `handle_uncaught_exception`
and `log_response`, thus logging the handler exception.
Current: (django/core/handlers/exception.py:89)
{{{
else:
signals.got_request_exception.send(sender=None, request=request)
response = handle_uncaught_exception(request,
get_resolver(get_urlconf()), sys.exc_info())
log_response(
'%s: %s', response.reason_phrase, request.path,
response=response,
request=request,
exc_info=sys.exc_info(),
)
}}}
Actually passing along the error for logging and building debug-response:
{{{
else:
try:
signals.got_request_exception.send(sender=None,
request=request)
except Exception as e:
# A new exception was raised by a receiver of
got_request_exception
# If we don't catch it, it will just recurse.
response = handle_uncaught_exception(request,
get_resolver(get_urlconf()), sys.exc_info())
log_response(
'%s: %s', response.reason_phrase, request.path,
response=response,
request=request,
exc_info=sys.exc_info(),
)
else:
response = handle_uncaught_exception(request,
get_resolver(get_urlconf()), sys.exc_info())
log_response(
'%s: %s', response.reason_phrase, request.path,
response=response,
request=request,
exc_info=sys.exc_info(),
)
}}}
Does any one have any pointers on how to improve this? This is the most
trivial solution, just to showcase the solution.
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:10>
Django
Reporter: patrakov@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Ludwig Kjellström):
* version: 1.6 => master
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:11>
Django
Reporter: patrakov@… | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: HTTP handling | Version: dev
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21777#comment:12>