Opened 12 years ago
Closed 12 years ago
#18482 closed Cleanup/optimization (fixed)
Cache backend API should close()
Reported by: | Russell Keith-Magee | 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
Change History (6)
comment:1 by , 12 years ago
follow-up: 3 comment:2 by , 12 years ago
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 by , 12 years ago
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 by , 12 years ago
Hey, is there anything that I should do to get this ticket closed/pull request merged?
comment:5 by , 12 years ago
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
Nope - the patch looks good to commit. Thanks!
comment:6 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
In 4c5cea70734fe8f4082c6a9bd8b26cf0a157ca78:
Merge pull request #218 from mgrouchy/ticket_18582
Fixed #18582 -- Added a no-op close to BaseCache
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.