Opened 13 years ago
Closed 13 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 , 13 years ago
follow-up: 3 comment:2 by , 13 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 , 13 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 , 13 years ago
Hey, is there anything that I should do to get this ticket closed/pull request merged?
comment:5 by , 13 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 , 13 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.