Opened 14 years ago

Closed 11 years ago

Last modified 9 years ago

#12250 closed Bug (wontfix)

process_view middleware prevents process_exception handling

Reported by: Michael Manfre Owned by: Michael Manfre
Component: Core (Other) Version: dev
Severity: Normal Keywords: middleware
Cc: Walter Doekes Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

If process_view() calls a view that raises and exception, it is handled by django.core.handlers.base.BaseHandler.handle_uncaught_exception() instead of being passed to the middleware for process_exception() handling.

Attachments (3)

django-base-handlers-view-middleware.patch (1.2 KB ) - added by Michael Manfre 14 years ago.
django-base-handlers-view-middleware-with-tests.diff (7.0 KB ) - added by Michael Manfre 13 years ago.
Patch against trunk rev 13959
12250.1.diff (5.8 KB ) - added by Ramiro Morales 13 years ago.
Patch modified and simplfied

Download all attachments as: .zip

Change History (21)

by Michael Manfre, 14 years ago

comment:1 by Jeremy Dunck, 14 years ago

Needs tests: set
Owner: changed from nobody to Jeremy Dunck
Patch needs improvement: set
Status: newassigned
Triage Stage: UnreviewedAccepted

comment:2 by Jeremy Dunck, 14 years ago

Keywords: middleware added
Owner: changed from Jeremy Dunck to nobody
Status: assignednew

comment:3 by Jeremy Dunck, 14 years ago

This really needs tests. It's a critical piece of code and there are lots of exception paths around there.

comment:4 by anonymous, 14 years ago

Needs tests: unset
Owner: changed from nobody to anonymous
Patch needs improvement: unset
Status: newassigned

by Michael Manfre, 13 years ago

Patch against trunk rev 13959

comment:5 by Michael Manfre, 13 years ago

milestone: 1.3
Owner: changed from anonymous to Michael Manfre
Status: assignednew

Updated the patch against trunk and hoping to get this in to 1.3

comment:6 by Ramiro Morales, 13 years ago

Triage Stage: AcceptedDesign decision needed

So the ticket originally was about exceptions generated in the view itself not being caught an processed by .process_exception().

But the code modification and accompanying tests proposed aren't about exceptions in the view but exceptions originated in a middleware process_view() method.

The first is no longer an issue (I suspect it was fixed in r14393 and r14398) and the second one still happens but I'm not sure the change of behavior in that case should happen. I'm adding a modified and simplified patch demonstrating both scenarios.

Because of that, I'm moving this to design decision needed.

Last edited 13 years ago by Ramiro Morales (previous) (diff)

by Ramiro Morales, 13 years ago

Attachment: 12250.1.diff added

Patch modified and simplfied

comment:7 by Matt McClanahan, 13 years ago

milestone: 1.3
Severity: Normal
Type: Bug

comment:8 by Aymeric Augustin, 12 years ago

UI/UX: unset

Change UI/UX from NULL to False.

comment:9 by Aymeric Augustin, 12 years ago

Easy pickings: unset

Change Easy pickings from NULL to False.

comment:10 by kevin1024, 12 years ago

I think the latest patch is missing the actual behavior modification. It seems to only have tests in it.

comment:11 by Walter Doekes, 11 years ago

Cc: Walter Doekes added
126	+        """
127	+        A <Middleware>.process_view() that generates an exception and another
128	+        middleware whose process_exception() provides a response. Result should
129	+        be that response.
130	+        """
131	+        settings.MIDDLEWARE_CLASSES = (
132	+            'regressiontests.base_handler.middleware.ProcessViewRaise',
133	+            'regressiontests.base_handler.middleware.ProcessExceptionResponse',
134	+        )

Well, I have a common scenario that would benefit from having the exception handler handle the process_view() exception:

django/middleware/csrf.py

calls

    request_csrf_token = request.POST.get('csrfmiddlewaretoken', '')
...
    self._load_post_and_files()
...
    return self._stream.read(*args, **kwargs)

IOError: error reading wsgi.input data

I'd love to ignore that particular IOError, and I can, when it's generated in the view. But I can't when it's generated in the CSRF process_view.

As for how: I'd expect it to be handled just as if it was an exception in the view itself. I.e. surround the _view_middleware calls with a try/except just like the "response = callback(...)" bit.

Regards,
Walter Doekes

comment:12 by Anssi Kääriäinen, 11 years ago

Another similar issue exists in process_request(). Consider case:

MIDDLEWARE_CLASSES = [
    'TransactionMiddleware',
    'SavesToDB',
    'RaisesException'
]

What happens is that TransactionMiddleware will open a transaction in process_request, savestodb will save to db and then RaisesException will raise an exception. The process_exceptions aren't called in this case. Finally process_response is called for every of the middlewares and thus TranscationMiddleware will commit the transaction. This is not good.

I see two solutions here - either we do the process_exception() dance for all middlewares for which process_request has been called (in process_view() case this means every middleware). Or, we decide that process_* should not ever raise an exception, and we go to panic mode if this happens. The panic mode would mean that we would try to release all resources (close connections etc) and then return a barebone 500 response.

To me the panic mode doesn't seem like a good idea.

I reported this in #19648. I marked that ticket as a duplicate of this one.

comment:13 by Aymeric Augustin, 11 years ago

This is almost the same problem as #16367 which I closed as wontfix.

Given the amount of discussion above, I'm not going to close as wontfix right now, but that's still my inclination.

comment:14 by Aymeric Augustin, 11 years ago

#12909 was closed as a duplicate.

comment:15 by Aymeric Augustin, 11 years ago

Resolution: wontfix
Status: newclosed

Overall, the current design of the request handler assumes that middleware methods won't raise exceptions. They must deal with their own exceptions. Exceptions raised by middleware will end up in handle_uncaught_exception and produce a 500.

This may have some drawbacks, but at least it's consistent. Aborting on the first exception also avoids the problem of deciding which middleware should or shouldn't run after the first exception.

I could consider a different behavior, but that would require a global proposal for consistent exception handling in middleware. It seems quite tricky to design something that is useful, consistent, and simple.

I'm going to close this ticket because it's a piecewise approach of the problem that introduces some inconsistency.

comment:16 by Walter Doekes, 11 years ago

Should I create a new ticket for the unhandled exception in the csrf middleware then? :)

comment:17 by Aymeric Augustin, 11 years ago

Yes please. Sorry, I missed that in the comments.

comment:18 by Christopher Martin, 9 years ago

"the current design of the request handler assumes that middleware methods won't raise exceptions. They must deal with their own exceptions. Exceptions raised by middleware will end up in handle_uncaught_exception and produce a 500."

That would be a great piece of information to include in https://docs.djangoproject.com/en/dev/topics/http/middleware/#writing-your-own-middleware

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