#26764 closed Cleanup/optimization (fixed)

Session.cycle_key() throws Exception if the session hasn't been accessed

Reported by: adam-iris Owned by: Adam Zapletal
Component: contrib.sessions Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A common use case I see is that a user changes some content then tries to view their changes on the website, but they get an older, previously-cached page.

Since the page-level cache key uses the session id, my solution is to call request.session.cycle_key(). To the user this is presented as a "hard reload" -- it clears the session key then redirects back to the current page, the page-level cache will miss and the user will get a newly-generated version.

However, this raised an exception:
'SessionStore' object has no attribute '_session_cache'

Upon inspection, it looks like SessionBase._session_cache doesn't get initialized except by calling SessionBase._session. But the cycle_key() method accesses _session_cache without ensuring that it's been initialized.

My workaround was to prepend a meaningless access call:

request.session.get('foo')
request.session.cycle_key()

This fixed the exception, but I think the proper fix is for cycle_key() to initialize (or more carefully check) _session_cache.

Change History (7)

comment:1 Changed 18 months ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization

comment:2 Changed 17 months ago by Adam Zapletal

Owner: changed from nobody to Adam Zapletal
Status: newassigned

comment:3 Changed 17 months ago by Adam Zapletal

PR is here: https://github.com/django/django/pull/7031

This is a simple fix, but I'm wondering why _session_cache isn't set to {} in __init__. Maybe a discussion for another ticket.

Thanks!

comment:4 Changed 17 months ago by Tim Graham

Did you try making that change and seeing if any tests fail? Might be cleaner than the proposed try/except.

comment:5 Changed 17 months ago by Adam Zapletal

Yeah, with self._session_cache = {} in __init__, the sessions_tests are OK, but instead of passing with two skips and an expected failure, they pass with one unexpected success.

However, the full Django test suite does not pass with this change. It had 511 failures and 55 errors...so something deeper is going on here. Unexpected!

I'd like to look into it more and understand contrib.sessions better in general as a separate effort.

Version 0, edited 17 months ago by Adam Zapletal (next)

comment:6 Changed 16 months ago by Adam Zapletal

Has patch: set

comment:7 Changed 16 months ago by Tim Graham <timograham@…>

Resolution: fixed
Status: assignedclosed

In 887f3d32:

Fixed #26764 -- Fixed Session.cycle_key() crash on unaccessed session.

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