Re: [Django] #12250: process_view middleware prevents process_exception handling
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by kevin1024):
I think the latest patch is missing the actual behavior modification. It
seems to only have tests in it.
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:10>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* cc: wdoekes (added)
Comment:
{{{
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
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by akaariai):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by aaugustin):
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:13>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by aaugustin):
#12909 was closed as a duplicate.
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Component: Core (Other) | Version: master
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => wontfix
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:15>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: wontfix
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by wdoekes):
Should I create a new ticket for the unhandled exception in the csrf
middleware then? :)
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:16>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: wontfix
Keywords: middleware | Triage Stage: Design
Has patch: 1 | decision needed
Needs tests: 0 | Needs documentation: 0
Easy pickings: 0 | Patch needs improvement: 0
| UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by aaugustin):
Yes please. Sorry, I missed that in the comments.
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:17>
Django
-------------------------------------+-------------------------------------
Reporter: manfre | Owner: manfre
Type: Bug | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: wontfix
Keywords: middleware | Triage Stage: Design
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by chris-martin):
"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
--
Ticket URL: <https://code.djangoproject.com/ticket/12250#comment:18>