core handlers do not catch middleware exceptions

[Gary Wilson]

This causes bare exceptions to be returned. To reproduce, just put a raise Exception statement in the process_request method of a middleware you are using. You will get a traceback like this in your browser:

Traceback (most recent call last):

  File "/home/gdub/bzr/django/upstream/django/core/servers/basehttp.py", line 277, in run
    self.result = application(self.environ, self.start_response)

  File "/home/gdub/bzr/django/upstream/django/core/servers/basehttp.py", line 619, in __call__
    return self.application(environ, start_response)

  File "/home/gdub/bzr/django/upstream/django/core/handlers/wsgi.py", line 205, in __call__
    response = self.get_response(request)

  File "/home/gdub/bzr/django/upstream/django/core/handlers/base.py", line 63, in get_response
    response = middleware_method(request)

  File "/home/gdub/bzr/django/upstream/django/middleware/common.py", line 29, in process_request
    raise Exception

Exception

If you put a raise Exception statement in the process_response method of a middleware you are using, then you will get a traceback like this in your browser:

Traceback (most recent call last):

  File "/home/gdub/bzr/django/upstream/django/core/servers/basehttp.py", line 277, in run
    self.result = application(self.environ, self.start_response)

  File "/home/gdub/bzr/django/upstream/django/core/servers/basehttp.py", line 619, in __call__
    return self.application(environ, start_response)

  File "/home/gdub/bzr/django/upstream/django/core/handlers/wsgi.py", line 209, in __call__
    response = middleware_method(request, response)

  File "/home/gdub/bzr/django/upstream/django/middleware/common.py", line 62, in process_response
    raise Exception

Exception

WSGI and ModPython handlers are both affected.

Discussion: ​http://groups.google.com/group/django-developers/browse_thread/thread/255b42846c7e49e9

I would think that we want these exceptions to be handled by the standard exception handling. Should they also be passed through process_exception middlewares? Currently only exceptions raised in views get passed through process_exception middlewares.

[Gary Wilson]

Patch notes:

• Added a handle_response method to BaseHandler to factor out duplicated code in ModPythonHandler, WSGIHandler, and ClientHandler.
• Replaced resolver temporary variable with a resolver method to ease refactoring of get_response method.
• Factored out request middleware, view processing, and response middleware into their own methods.
• Turned get_response method into a more generic method that will apply the standard exception handling to any passed method.
• Made test Client class configurable for reraising request-handling exceptions or not.
• Added tests for making sure that exceptions raised in various middleware are handled properly.
• Added tests to test that the correct exceptions bubble up from middleware with errors.

[Karen Tracey <kmtracey@…>]

Marked #6268 as a dup.

[Karen Tracey <kmtracey@…>]

#6269 also marked as a dup.

[eibaan <eibaan ät googlemail>]

The patch fixes #6268 and #6269 but it does not apply cleanly. I've prepared a new one which hopefully does. It also does not test the error case of #6269 so I took the freedom to add another test case.

[Malcolm Tredinnick]

Gary,

I've reviewed this fairly carefully (I was hoping to check it in) and it's fine. The only minor change I'd like to make is move the resolve() method out of the class and into the handlers.base namespace. It might be useful externally (and it fits better here than in urlresolvers.py, since that is request-agnostic). So just remove the "self" and dedent, etc.

The major problem is the tests don't pass (in either version of the patch). The template tests show failures in all the "url" template tags. I've poked around a bit, but I can't see what's going on there. That needs to be fixed, but I can't keep working on this right now.

So, if you can work out the test problem (which probably suggests somebody bigger going wrong and move resolve() out of the class it's in, feel free to commit.

It's probably worth making a note in "backwards incompatible changes" about the fact that get_response() has changed. It may turn out that somebody has created their own handler subclass and are calling that directly. Just a quick pointer to use handle_request() should be sufficient; it's hardly a major issue.

[Malcolm Tredinnick]

(Oh, and if middleware exceptions went through the exception middleware path that would be cool, too. Every successful response goes through the response middleware path, regardless of whether it's generated by a view or a request middleware, so let's be consistent on the exception path, too.)

[Gary Wilson]

Replying to mtredinnick:

I've reviewed this fairly carefully (I was hoping to check it in) and it's fine. The only minor change I'd like to make is move the resolve() method out of the class and into the handlers.base namespace. It might be useful externally (and it fits better here than in urlresolvers.py, since that is request-agnostic). So just remove the "self" and dedent, etc.

Sounds good, done.

The major problem is the tests don't pass (in either version of the patch). The template tests show failures in all the "url" template tags. I've poked around a bit, but I can't see what's going on there. That needs to be fixed, but I can't keep working on this right now.

So, if you can work out the test problem (which probably suggests somebody bigger going wrong and move resolve() out of the class it's in, feel free to commit.

I'm not seeing any failing tests here. Can someone please confirm this.

It's probably worth making a note in "backwards incompatible changes" about the fact that get_response() has changed. It may turn out that somebody has created their own handler subclass and are calling that directly. Just a quick pointer to use handle_request() should be sufficient; it's hardly a major issue.

Agreed.

[Gary Wilson]

updated patch, 6094.3.diff:

• resolve method moved to a function of handles.base.
• Added a couple docstrings, including ascii chart of ​Jacob's request-handling image from OSCON 2007.

[Gary Wilson]

Replying to mtredinnick:

(Oh, and if middleware exceptions went through the exception middleware path that would be cool, too. Every successful response goes through the response middleware path, regardless of whether it's generated by a view or a request middleware, so let's be consistent on the exception path, too.)

Yeah, I was wondering if this should happen or not. Probably should be its own ticket and would need some thought about if or how it would affect existing middleware, such as TransactionMiddleware.

[Gary Wilson]

My only other concern with this is that I haven't tested in a ModPython setup. I don't think there would be problems, but just would like to have some confirmation.

[Gary Wilson]

latest patch

[George Vilches]

Patch cleanup to work with r7320, no other changes made from previous.

[George Vilches]

I just tested the updated patch with mod_wsgi and mod_python, both did what was expected (threw the proper 500 built-in page).

[Gary Wilson]

marked #2481 as a dup, but there was an interesting idea for a configurable exception handler in the comments there.

[Jacob]

Pushing post-1.0: at this point this will destabilize the core handler a bit too much for my liking. We've lived with this annoyance for a while; we can deal with it for a bit longer still.

[(none)]

Milestone post-1.0 deleted

[groks]

Replying to jacob:

Pushing post-1.0: at this point this will destabilize the core handler a bit too much for my liking. We've lived with this annoyance for a while; we can deal with it for a bit longer still.

It's now post 1.0 (and post 1.1, I guess). Any chance you can take another look at this?

[Ivan Sagalaev]

Minimal patch and test

[Ivan Sagalaev]

I've made an alternative patch that is much less involving than the previous one. Basically it just shoves request middleware handling under try statements in BaseHandler.

Refactoring of the request part is a good thing by itself but I think it holds this actual bug from fixing. It can be done after it's closed.

[Adrian Holovaty]

(In [12165]) Fixed #6094 -- Middleware exceptions are now caught by the core handler. Thanks, isagalaev

[Ivan Sagalaev]

Patch fixing broken tests

[Adrian Holovaty]

(In [12186]) Fixed #6094 again -- fixed broken unit tests. Thanks, isagalaev

[Andrey Golovizin]

After [12186] it is broken again - reopening.

[Karen Tracey]

Please open a new ticket describing the current problem. It's hard to determine what is meant by "broken again"; a fresh problem that actually describes the current situation is much better than reopening a ticket with 2 years of history to dig through to figure out what you might be trying to report.

[Andrey Golovizin]

Do you want me to copy&paste the description of this ticket into a new one? Fine.

[Karen Tracey]

Sigh. I'd like a description of what made you even go find this ticket. I tried the first thing in the description -- putting an exception in a middleware process_request -- and got a debug page, not a bare exception. So something has been fixed, and it would help if you could explain exactly what you see that has not been fixed.

[Andrey Golovizin]

OK, created #13090.