Opened 16 years ago

Closed 14 years ago

Last modified 14 years ago

#6094 closed (fixed)

core handlers do not catch middleware exceptions

Reported by: Gary Wilson Owned by: Gary Wilson
Component: HTTP handling Version: dev
Severity: Keywords:
Cc: jdunck@…, cmawebsite@…, gav@…, Trevor Caira, mpjung@…, me@…, Maniac@…, sdeasey@…, Carl Meyer, Gonzalo Saavedra, Gert Van Gool, Andrey Golovizin Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (7)

6094.diff (19.9 KB ) - added by Gary Wilson 16 years ago.
6094-2.diff (21.2 KB ) - added by eibaan <eibaan ät googlemail> 16 years ago.
6094.3.diff (22.6 KB ) - added by Gary Wilson 16 years ago.
6094.2008-02-01.diff (23.8 KB ) - added by Gary Wilson 16 years ago.
latest patch
http_refactor_r7320.patch (23.8 KB ) - added by George Vilches 16 years ago.
Patch cleanup to work with r7320, no other changes made from previous.
6094-minimal.diff (4.8 KB ) - added by Ivan Sagalaev 14 years ago.
Minimal patch and test
6094.fix-broken-test.diff (2.3 KB ) - added by Ivan Sagalaev 14 years ago.
Patch fixing broken tests

Download all attachments as: .zip

Change History (43)

comment:1 by Simon G <dev@…>, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:2 by Gary Wilson, 16 years ago

Owner: changed from nobody to Gary Wilson

by Gary Wilson, 16 years ago

Attachment: 6094.diff added

comment:3 by Gary Wilson, 16 years ago

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.

comment:4 by Karen Tracey <kmtracey@…>, 16 years ago

Marked #6268 as a dup.

comment:5 by Karen Tracey <kmtracey@…>, 16 years ago

#6269 also marked as a dup.

comment:6 by eibaan <eibaan ät googlemail>, 16 years ago

Has patch: set

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.

by eibaan <eibaan ät googlemail>, 16 years ago

Attachment: 6094-2.diff added

comment:7 by Malcolm Tredinnick, 16 years ago

Patch needs improvement: set
Triage Stage: Design decision neededAccepted

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.

comment:8 by Malcolm Tredinnick, 16 years ago

(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.)

comment:9 by Collin Anderson, 16 years ago

Cc: cmawebsite@… added

in reply to:  7 comment:10 by Gary Wilson, 16 years ago

Patch needs improvement: unset

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.

by Gary Wilson, 16 years ago

Attachment: 6094.3.diff added

comment:11 by Gary Wilson, 16 years ago

updated patch, 6094.3.diff:

in reply to:  8 comment:12 by Gary Wilson, 16 years ago

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.

comment:13 by Gary Wilson, 16 years ago

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.

by Gary Wilson, 16 years ago

Attachment: 6094.2008-02-01.diff added

latest patch

comment:14 by George Vilches, 16 years ago

Cc: gav@… added

by George Vilches, 16 years ago

Attachment: http_refactor_r7320.patch added

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

comment:15 by George Vilches, 16 years ago

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

comment:16 by Gary Wilson, 16 years ago

milestone: 1.0
Status: newassigned

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

comment:17 by anonymous, 16 years ago

Cc: Trevor Caira added

comment:18 by Michael P. Jung, 16 years ago

Cc: mpjung@… added

comment:19 by Jacob, 16 years ago

milestone: 1.0post-1.0

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.

comment:20 by Jeff Balogh, 15 years ago

Cc: me@… added

comment:21 by Ivan Sagalaev, 15 years ago

Cc: Maniac@… added

comment:22 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

in reply to:  19 comment:23 by groks, 15 years ago

Cc: sdeasey@… added

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?

comment:24 by Carl Meyer, 15 years ago

Cc: Carl Meyer added

comment:25 by Gonzalo Saavedra, 15 years ago

Cc: Gonzalo Saavedra added

comment:26 by Jeremy Dunck, 15 years ago

Cc: jdunck@… added

comment:27 by Gert Van Gool, 15 years ago

Cc: Gert Van Gool added

by Ivan Sagalaev, 14 years ago

Attachment: 6094-minimal.diff added

Minimal patch and test

comment:28 by Ivan Sagalaev, 14 years ago

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.

comment:29 by Adrian Holovaty, 14 years ago

Resolution: fixed
Status: assignedclosed

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

by Ivan Sagalaev, 14 years ago

Attachment: 6094.fix-broken-test.diff added

Patch fixing broken tests

comment:30 by Adrian Holovaty, 14 years ago

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

comment:31 by Andrey Golovizin, 14 years ago

Resolution: fixed
Status: closedreopened

After [12186] it is broken again - reopening.

comment:32 by Andrey Golovizin, 14 years ago

Cc: Andrey Golovizin added

comment:33 by Karen Tracey, 14 years ago

Resolution: fixed
Status: reopenedclosed

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.

comment:34 by Andrey Golovizin, 14 years ago

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

comment:35 by Karen Tracey, 14 years ago

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.

comment:36 by Andrey Golovizin, 14 years ago

OK, created #13090.

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