Opened 12 years ago

Closed 7 years ago

#3881 closed Bug (fixed)

Don't mask server exception with session save exception

Reported by: hauserx@… Owned by: Adrian Holovaty
Component: contrib.sessions Version: 1.3-beta
Severity: Normal Keywords:
Cc: joel@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


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\", 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.

Attachments (2)

session_save.patch (1.1 KB) - added by hauserx@… 12 years ago.
Save session only when no exception during request processing
session_save.patch (1.0 KB) - added by anonymous 12 years ago.
Don't save session if an exception occured

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by anonymous

Version: 0.95SVN

Changed 12 years ago by hauserx@…

Attachment: session_save.patch added

Save session only when no exception during request processing

comment:2 Changed 12 years ago by Simon G. <dev@…>

Has patch: set
Triage Stage: UnreviewedAccepted

Changed 12 years ago by anonymous

Attachment: session_save.patch added

Don't save session if an exception occured

comment:3 Changed 12 years ago by Adrian Holovaty

Resolution: invalid
Status: newclosed

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

comment:4 Changed 8 years ago by terry_n_brown@…

Component: Core (Other)contrib.sessions
Easy pickings: unset
Resolution: invalid
Severity: Normal
Status: closedreopened
Type: Bug
UI/UX: unset
Version: SVN1.3-beta

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.

comment:5 Changed 7 years ago by myusuf3

Patch needs improvement: set

Patch no longer applies.

comment:6 Changed 7 years ago by Joel Cross

Cc: joel@… added

comment:7 Changed 7 years ago by 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():

The above would be done in sessions/backends/ (and maybe in 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.

Last edited 7 years ago by Anssi Kääriäinen (previous) (diff)

comment:8 Changed 7 years ago by Anssi Kääriäinen

Patch needs improvement: unset

I created a branch implementing the "skip save on 500" idea.

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.

comment:9 Changed 7 years ago by Anssi Kääriäinen <akaariai@…>

Resolution: fixed
Status: reopenedclosed

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.

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