Code

Opened 3 years ago

Closed 2 years ago

#16708 closed Cleanup/optimization (fixed)

Session backend cached_db's exists() does not check cache before database

Reported by: jpvale@… Owned by: aaugustin
Component: contrib.sessions Version: master
Severity: Normal Keywords: session cache cached_db
Cc: jvale Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

In the cached_db session backend, the exists() method does not check the cache before hitting the database. It should first hit the cache, and only query the database if the session is not already cached.

Attachments (2)

cached_db_exists_check_cache.diff (517 bytes) - added by jpvale@… 3 years ago.
cached_db_exists_check_cache_v2.diff (2.5 KB) - added by jvale 3 years ago.

Download all attachments as: .zip

Change History (7)

Changed 3 years ago by jpvale@…

comment:1 Changed 3 years ago by aaugustin

  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

This file was never modified since it was checked in at r9727.

The current behavior may be an omission that went through the reviews unnoticed, or an implementation choice: it avoids loading the contents of the session just to check if it exists. The comments on #6791 don't mention it.

Since Django's cache API doesn't provide cache.exists(key), there's no way to check if a session exists in the cache without getting the contents of the session. It's impossible to optimize this for the general case, because it depends on the speed/scalability of the database, the speed/scalability of the cache, and the size of the session.

Since sessions tend to be small, I'm +0 on this change.

If it's accepted, the patch would need tests.

Changed 3 years ago by jvale

comment:2 Changed 3 years ago by jvale

  • Cc jvale added

I'm the ticket creator, thought I'd create an account here.

Added a second version of the patch, this time with tests.

comment:3 Changed 3 years ago by Alex

  • Triage Stage changed from Design decision needed to Accepted

comment:4 Changed 2 years ago by aaugustin

  • Owner changed from nobody to aaugustin

comment:5 Changed 2 years ago by aaugustin

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

I fixed that problem in c11f9c3193901215ec79732af6ab6c66b3c1c2ba.

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.