Opened 8 years ago

Closed 5 years ago

Last modified 5 years ago

#6094 closed (fixed)

core handlers do not catch middleware exceptions

Reported by: gwilson Owned by: gwilson
Component: HTTP handling Version: master
Severity: Keywords:
Cc: jdunck@…, cmawebsite@…, gav@…, trevor, mpjung@…, me@…, Maniac@…, sdeasey@…, carljm, gonz, gvangool, EroSennin 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 gwilson 8 years ago.
6094-2.diff (21.2 KB) - added by eibaan <eibaan ät googlemail> 8 years ago.
6094.3.diff (22.6 KB) - added by gwilson 8 years ago.
6094.2008-02-01.diff (23.8 KB) - added by gwilson 8 years ago.
latest patch
http_refactor_r7320.patch (23.8 KB) - added by gav 7 years ago.
Patch cleanup to work with r7320, no other changes made from previous.
6094-minimal.diff (4.8 KB) - added by isagalaev 6 years ago.
Minimal patch and test
6094.fix-broken-test.diff (2.3 KB) - added by isagalaev 6 years ago.
Patch fixing broken tests

Download all attachments as: .zip

Change History (43)

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

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 8 years ago by gwilson

  • Owner changed from nobody to gwilson

Changed 8 years ago by gwilson

comment:3 Changed 8 years ago by gwilson

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 8 years ago by Karen Tracey <kmtracey@…>

Marked #6268 as a dup.

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

#6269 also marked as a dup.

comment:6 Changed 8 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 8 years ago by eibaan <eibaan ät googlemail>

comment:7 follow-up: Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

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 follow-up: Changed 8 years ago by 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.)

comment:9 Changed 8 years ago by CollinAnderson

  • Cc cmawebsite@… added

comment:10 in reply to: ↑ 7 Changed 8 years ago by gwilson

  • 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 8 years ago by gwilson

comment:11 Changed 8 years ago by gwilson

updated patch, 6094.3.diff:

comment:12 in reply to: ↑ 8 Changed 8 years ago by gwilson

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

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

latest patch

comment:14 Changed 8 years ago by gav

  • Cc gav@… added

Changed 7 years ago by gav

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

comment:15 Changed 7 years ago by gav

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 7 years ago by gwilson

  • milestone set to 1.0
  • Status changed from new to assigned

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

comment:17 Changed 7 years ago by anonymous

  • Cc trevor added

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

  • Cc mpjung@… added

comment:19 follow-up: Changed 7 years ago by jacob

  • milestone changed from 1.0 to post-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 7 years ago by jbalogh

  • Cc me@… added

comment:21 Changed 7 years ago by isagalaev

  • Cc Maniac@… added

comment:22 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:23 in reply to: ↑ 19 Changed 6 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 6 years ago by carljm

  • Cc carljm added

comment:25 Changed 6 years ago by gonz

  • Cc gonz added

comment:26 Changed 6 years ago by jdunck

  • Cc jdunck@… added

comment:27 Changed 6 years ago by gvangool

  • Cc gvangool added

Changed 6 years ago by isagalaev

Minimal patch and test

comment:28 Changed 6 years ago by isagalaev

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 6 years ago by adrian

  • Resolution set to fixed
  • Status changed from assigned to closed

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

Changed 6 years ago by isagalaev

Patch fixing broken tests

comment:30 Changed 6 years ago by adrian

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

comment:31 Changed 5 years ago by EroSennin

  • Resolution fixed deleted
  • Status changed from closed to reopened

After [12186] it is broken again - reopening.

comment:32 Changed 5 years ago by EroSennin

  • Cc EroSennin added

comment:33 Changed 5 years ago by kmtracey

  • Resolution set to fixed
  • Status changed from reopened to closed

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 5 years ago by EroSennin

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

comment:35 Changed 5 years ago by kmtracey

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 5 years ago by EroSennin

OK, created #13090.

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