Opened 9 years ago

Closed 7 years ago

Last modified 7 years ago

#6094 closed (fixed)

core handlers do not catch middleware exceptions

Reported by: Gary Wilson Owned by: Gary Wilson
Component: HTTP handling Version: master
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: UI/UX:

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

Download all attachments as: .zip

Change History (43)

comment:1 Changed 9 years ago by Simon G <dev@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 9 years ago by Gary Wilson

Owner: changed from nobody to Gary Wilson

Changed 9 years ago by Gary Wilson

Attachment: 6094.diff added

comment:3 Changed 9 years ago by 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.

comment:4 Changed 9 years ago by Karen Tracey <kmtracey@…>

Marked #6268 as a dup.

comment:5 Changed 9 years ago by Karen Tracey <kmtracey@…>

#6269 also marked as a dup.

comment:6 Changed 9 years ago by eibaan <eibaan ät googlemail>

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.

Changed 9 years ago by eibaan <eibaan ät googlemail>

Attachment: 6094-2.diff added

comment:7 Changed 9 years ago by Malcolm Tredinnick

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 Changed 9 years ago by 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.)

comment:9 Changed 9 years ago by Collin Anderson

Cc: cmawebsite@… added

comment:10 in reply to:  7 Changed 9 years ago by Gary Wilson

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.

Changed 9 years ago by Gary Wilson

Attachment: 6094.3.diff added

comment:11 Changed 9 years ago by Gary Wilson

updated patch, 6094.3.diff:

comment:12 in reply to:  8 Changed 9 years ago by 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.

comment:13 Changed 9 years ago by 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.

Changed 9 years ago by Gary Wilson

Attachment: 6094.2008-02-01.diff added

latest patch

comment:14 Changed 9 years ago by George Vilches

Cc: gav@… added

Changed 9 years ago by George Vilches

Attachment: http_refactor_r7320.patch added

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

comment:15 Changed 9 years ago by 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).

comment:16 Changed 8 years ago by Gary Wilson

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 Changed 8 years ago by anonymous

Cc: Trevor Caira added

comment:18 Changed 8 years ago by Michael P. Jung

Cc: mpjung@… added

comment:19 Changed 8 years ago by Jacob

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 Changed 8 years ago by Jeff Balogh

Cc: me@… added

comment:21 Changed 8 years ago by Ivan Sagalaev

Cc: Maniac@… added

comment:22 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:23 in reply to:  19 Changed 8 years ago by groks

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 Changed 7 years ago by Carl Meyer

Cc: Carl Meyer added

comment:25 Changed 7 years ago by Gonzalo Saavedra

Cc: Gonzalo Saavedra added

comment:26 Changed 7 years ago by Jeremy Dunck

Cc: jdunck@… added

comment:27 Changed 7 years ago by Gert Van Gool

Cc: Gert Van Gool added

Changed 7 years ago by Ivan Sagalaev

Attachment: 6094-minimal.diff added

Minimal patch and test

comment:28 Changed 7 years ago by 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.

comment:29 Changed 7 years ago by Adrian Holovaty

Resolution: fixed
Status: assignedclosed

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

Changed 7 years ago by Ivan Sagalaev

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

Patch fixing broken tests

comment:30 Changed 7 years ago by Adrian Holovaty

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

comment:31 Changed 7 years ago by Andrey Golovizin

Resolution: fixed
Status: closedreopened

After [12186] it is broken again - reopening.

comment:32 Changed 7 years ago by Andrey Golovizin

Cc: Andrey Golovizin added

comment:33 Changed 7 years ago by Karen Tracey

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 Changed 7 years ago by Andrey Golovizin

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

comment:35 Changed 7 years ago by 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.

comment:36 Changed 7 years ago by Andrey Golovizin

OK, created #13090.

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