Code

Opened 7 years ago

Closed 21 months ago

#3881 closed Bug (fixed)

Don't mask server exception with session save exception

Reported by: hauserx@… Owned by: adrian
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

Description

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.

Attachments (2)

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

Download all attachments as: .zip

Change History (11)

comment:1 Changed 7 years ago by anonymous

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Version changed from 0.95 to SVN

Changed 7 years ago by hauserx@…

Save session only when no exception during request processing

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

  • Has patch set
  • Triage Stage changed from Unreviewed to Accepted

Changed 7 years ago by anonymous

Don't save session if an exception occured

comment:3 Changed 7 years ago by adrian

  • Resolution set to invalid
  • Status changed from new to closed

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 3 years ago by terry_n_brown@…

  • Component changed from Core (Other) to contrib.sessions
  • Easy pickings unset
  • Resolution invalid deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to Bug
  • UI/UX unset
  • Version changed from SVN to 1.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 2 years ago by myusuf3

  • Patch needs improvement set

Patch no longer applies.

comment:6 Changed 23 months ago by ukch

  • Cc joel@… added

comment:7 Changed 22 months ago by akaariai

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.

Last edited 22 months ago by akaariai (previous) (diff)

comment:8 Changed 22 months ago by akaariai

  • Patch needs improvement unset

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.

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

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.