Don't mask server exception with session save exception

[hauserx@…]

Now session is saved even when the server callback method throws an exception. When callback exception is caused by database, sometimes save of session in SessionMiddleware fails and a new exception is thrown. This new exception is presented instead of technical_500_response.

This new stacktrace gives no information of what is the real cause of the exception, the resulting output may end with something that do not give to much information, like that for postgres:

File "C:\Programs\Python\Lib\site-packages\Django-0.95.1-py2.5.egg\django\db\backends\postgresql_psycopg2\base.py", line 46, in cursor

cursor.execute("SET TIME ZONE %s", [settings.TIME_ZONE])

ProgrammingError: current transaction is aborted, commands ignored until end of transaction block

Question which can be asked is: Should the session state be saved when an exception is thrown from the callback method? In my opinion it shouldn't be, because it means that something unexpected happen.

I will prepare a patch that will save session only if there was no uncaught exception if it is acceptable solution.

[hauserx@…]

Save session only when no exception during request processing

[anonymous]

Don't save session if an exception occured

[Adrian Holovaty]

When I applied this patch, I could no longer log into the Django admin. It was as if my session wasn't being saved.

[terry_n_brown@…]

I'm still seeing exactly this problem in 1.3 beta. I applied the patch, which is very simple, by hand, and it worked for me, logging in to admin was not impacted.

Also, closing the ticket because of an invalid patch doesn't make sense, the ticket itself needs to be invalid.

[myusuf3]

Patch no longer applies.

[Anssi Kääriäinen]

I just got hit by this issue. My initial though for fix was to add connection.can_run_queries() method, so that one could do:

if connection.can_run_queries():
    session.save()

The above would be done in sessions/backends/db.py (and maybe in cached_db.py also).

Then I saw this ticket and now I am wondering if the approach taken in this ticket is better. The main question is if we want to save the session at all when an exception occurs?

I will go with the can_run_queries() approach if no other opinions are given. The reason is that it should be somewhat simple to do and backwards compatible.

EDIT: Just checked the original patch more carefully, and it is totally bogus. The middleware instance is global, not per response, and thus one can not use "self" to pass data. Currently if you hit one exception, then after that your sessions will not be saved. So, I think I will go with the connection.can_run_queries() method.

Another approach would be to check the status code of the response - if it is 500, then don't save session. This actually seems like a good approach - if there is an internal error, then it seems like saving the session should not be done.

[Anssi Kääriäinen]

I created a branch implementing the "skip save on 500" idea. ​https://github.com/akaariai/django/tree/ticket_3881

My reasoning is that in 500 situations the session save is more likely to fail than in other situations. In addition the save could easily lead into saving incomplete data - the processing of the request likely ended due to unexpected error, and thus it is possible that only portion of the intended session updates were done.

I will not commit this patch if I don't get an OK for this patch from another core dev. I will make a short post to django-developers about this.

[Anssi Kääriäinen <akaariai@…>]

In [aeda55e6bffc3cfbf53698af398a19c1a0f02d46]:

Fixed #3881 -- skip saving session when response status is 500

Saving session data is somewhat likely to lead into error when the
status code is 500. It is guaranteed to lead into error if the reason
for the 500 code is query error on PostgreSQL.