Code

Opened 2 years ago

Closed 20 months ago

#18482 closed Cleanup/optimization (fixed)

Cache backend API should close()

Reported by: russellm Owned by: nobody
Component: Core (Cache system) Version: 1.4
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

At present, only the memcached cache backends have a close() method. This means that you can't reliably call close() on a cache -- you need to protect it with "hasattr(cache, 'close')".

Django's own code even does this, in the end-request signal handler.

The cache backend API should include a no-op close() method by default, matching the API from the memcached backend.

The close() method (and the end-request handler) were added in r8418

Attachments (0)

Change History (6)

comment:1 Changed 2 years ago by mgrouchy

I added a pull request for this issue here: https://github.com/django/django/pull/218

I'm not sure if there any extra test requirements for this, but if there are, I would appreciate some guidance there and will update the pull request appropriately.

comment:2 follow-up: Changed 2 years ago by russellm

  • Needs tests set
  • Patch needs improvement set

Thanks for the patch!

The test requirement here is really just to exercise the new API point. There is an existing suite of cache tests in regressiontests/cache that use every function on the cache API; add a new test that calls close to the BaseCacheTest, and that method will be invoked for every cache backend.

comment:3 in reply to: ↑ 2 Changed 2 years ago by mgrouchy

Replying to russellm:

Thanks for the patch!

No Problem!

The test requirement here is really just to exercise the new API point. There is an existing suite of cache tests in regressiontests/cache that use every function on the cache API; add a new test that calls close to the BaseCacheTest, and that method will be invoked for every cache backend.

I added a Test, basically ensuring close exists and then using the endpoint.

Let me know if anything needs to be changed there. Pull request has been updated.

comment:4 Changed 2 years ago by mgrouchy

Hey, is there anything that I should do to get this ticket closed/pull request merged?

comment:5 Changed 2 years ago by russellm

  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Accepted to Ready for checkin

Nope - the patch looks good to commit. Thanks!

comment:6 Changed 20 months ago by aaugustin

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

In 4c5cea70734fe8f4082c6a9bd8b26cf0a157ca78:

Merge pull request #218 from mgrouchy/ticket_18582

Fixed #18582 -- Added a no-op close to BaseCache

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.