Skip session save on 500 responses

[Anssi Kääriäinen]

Currently Django's session middleware saves sessions unconditionally
in process_response. I have created a patch which skips session save
on 500 responses. Reasons for this:
1. When saving the session after PostgreSQL query error into database
backed session store, the save is guaranteed to fail due to aborted
transaction error. The save error will mask the original error.
2. The session.save() will be cancelled by transaction middleware if
that is installed.
3. Saving the session in 500 situations can be argued to be non-
wanted behaviour. The request processing likely ended due to
unexpected error. If so, it is also possible that the changes to
session aren't valid, or are incomplete.

Now, this is clearly a change to existing behaviour. Should this be
considered backwards incompatible or is this change not wanted at all?
If that is the case I will create a patch targeting #1 above
specifically.

- Anssi

Ticket: https://code.djangoproject.com/ticket/3881
Patch: https://github.com/akaariai/django/tree/ticket_3881

[Luke Plant]

On 05/07/12 16:25, Anssi Kääriäinen wrote:
> Currently Django's session middleware saves sessions unconditionally
> in process_response. I have created a patch which skips session save
> on 500 responses. Reasons for this:
> 1. When saving the session after PostgreSQL query error into database
> backed session store, the save is guaranteed to fail due to aborted
> transaction error. The save error will mask the original error.
> 2. The session.save() will be cancelled by transaction middleware if
> that is installed.
> 3. Saving the session in 500 situations can be argued to be non-
> wanted behaviour. The request processing likely ended due to
> unexpected error. If so, it is also possible that the changes to
> session aren't valid, or are incomplete.
>
> Now, this is clearly a change to existing behaviour. Should this be
> considered backwards incompatible or is this change not wanted at all?
> If that is the case I will create a patch targeting #1 above
> specifically.

I agree it should be changed, and I would regard it as a bug fix, but
make a note of it in the 1.5 release notes nonetheless.

Regards,

Luke


--
"Pessimism: Every dark cloud has a silver lining, but lightning
kills hundreds of people each year trying to find it." (despair.com)

Luke Plant || http://lukeplant.me.uk/

[Florian Apolloner]

On Friday, July 6, 2012 12:02:56 AM UTC+2, Luke Plant wrote:

I agree it should be changed, and I would regard it as a bug fix, but
make a note of it in the 1.5 release notes nonetheless.

+1

Cheers,
Florian

[Don Spaulding]

I was recently bit by this when I had an exception being thrown for a non-existent column.  Instead of seeing the psycopg2 exception that I would expect, I was continually seeing the "current transaction is aborted" exception.

Since I had figured out what the initial exception was, I finally dug in a little bit to see why I wasn't getting the traceback for it.  I expected to find some middleware of my own creation catching (and swallowing) the initial exception.  However, I ended up tracing my view and was surprised to see the exception bubbled all the way up to django/core/handlers/base.py.

I found this ticket and wondered if the solution in it was the right approach, or more accurately, whether there are two problems worth fixing here.  Specifically, I'm wondering why, when given an unhandled exception in the view callback and then another one in a response middleware, Django should choose to display the middleware exception.  Obviously, the current behavior of always running response middleware even in the face of an unhandled exception is intended.

I propose that Django keep a reference to the original uncaught exception, and return that one to the client even if a response middleware throws an uncaught exception as well.  I'm much less concerned with whether or not the session.save() succeeds or fails than I am with what the original exception was.  If I'm told what the original exception was, I can stop aborting transactions, and the session.save() fixes itself.  It may still be worth it to guard the call to session.save() with a connection.can_run_queries() conditional, but to me, that seems less important than having BaseHandler.get_response() show me the debug 500 page on the "right" exception.

First exception is caught by: https://github.com/django/django/blob/master/django/core/handlers/base.py#L178

session.save() bug is later caught by: https://github.com/django/django/blob/master/django/core/handlers/base.py#L192

So, am I way off base with this?  Is it worth opening a separate ticket?

 

[Anssi Kääriäinen]

> First exception is caught by:https://github.com/django/django/blob/master/django/core/handlers/bas...
> session.save() bug is later caught by:https://github.com/django/django/blob/master/django/core/handlers/bas...

>
> So, am I way off base with this?  Is it worth opening a separate ticket?

I have updated the branch [https://github.com/akaariai/django/tree/
ticket_3881], I think it now has all the necessary doc changes. If
somebody is kind enough to double-check the patch, that would be
great. If that somebody is a committer, committing the code at the
same time is fine for me. If no further feedback, I will commit the
patch in a day or two.

As for showing the original exception if a middleware generates an
exception, something along those lines sounds good. However, hiding
the middleware exception can be as bad as hiding the original
exception. So, maybe there should be a message telling that there was
an exception in the view, and then an exception in the middleware,
too. The latest (that is, the middleware's exception) should be shown
in full, as that is the first thing that should be debugged. There is
no way to get the middleware exception's traceback if that is not the
default, but one can get the original exception by just disabling the
problematic middleware.

- Anssi

[Luke Plant]

On 09/07/12 20:07, Anssi Kääriäinen wrote:

> I have updated the branch [https://github.com/akaariai/django/tree/
> ticket_3881], I think it now has all the necessary doc changes. If
> somebody is kind enough to double-check the patch, that would be
> great. If that somebody is a committer, committing the code at the
> same time is fine for me. If no further feedback, I will commit the
> patch in a day or two.

LGTM, I'll leave to you to commit.