OFS.Cache remixed by DJ Lessbugs

[Jamie Heilman]

I've refactored OFS/Cache.py to include a few featurettes I needed for a new CacheManager implementation and squashed a troup of bugs in the process. Here's the summary:

>Bugs squashed --
> Cacheable.ZCacheable_getURL unprotected
> could be called ttw
> CacheManager.manage_{afterAdd,beforeDelete} unprotected
> could be called by restricted code
> CacheManager.ZCacheManager_locate object hierarchy exposure
> under atypical conditions (Change cache settings
> permission given without View management screens
> given in kind, maybe a "lord of the cache"
> scenario? [obviously a very minor problem]) this
> method could be used to examine an object hierarchy
> which wouldn't otherwise be visible to a user

>Noteworthy improvements --
> removed all 4 bare except clauses in Cache.py
> ~100 lines less code than before
> uses declarative security model throughout

>Featurettes introduced --
> better exception interface
> support for weakref.proxy wrapped Cache instances in
> Cacheable mix-in

The case for a new exception interface:

The current Cache.py uses bare except clauses to ignore and log any exceptions raised by Cache.ZCache_{set,get,invalidate}. Apart from the more global problems caused by bare except statements, this implementation also dumped a full traceback into the logs everytime something innocuous happened (eg. when a RAM Cache Manager got handed unpicklable data) when the exception value alone would suffice in those conditions. Instead of swallowing exceptions, the refactored version defines a new CacheException class, which may be subclassed by Cache implementations, and only caches and logs exceptions if they are derived from that class. This gives the Cache author an API for logging non-terminal, but inappropriate cache activity. (eg. being handed data which is uncacheable for whatever reason)

The case for support weakref ProxyTypes:

When resetting the properties of an active RAM Cache the RAM Cache Manager simply makes some adjustments and lets the cache adjust itself naturally at the given cleanup interval. That works great for the RAM Cache product, but it doesn't work great for all cache products. There are times when resetting the properties of an active cache could mean invoking expensive thread locks to do a mass cleanup of the internal data structures. (Generally this will occur anytime you change a property which then means having to examine or change every object currently in the cache.) To avoid the overhead of a long-running cache scrubing its can be entirely acceptable to just throw the cache out entirely and start over, and weakref proxy objects give us just the mechanism to do this painlessly. The refactored Cache.py allows for CacheManager.ZCacheManager_getCache to return a weakref.proxy wrapped Cache object, or a direct reference to a Cache object (thus maintaining backwards compability). If a Cacheable object attempts a ZCacheable_* operation and it raises a ReferenceError it will simply invalidate its volatile reference to the current Cache so that the next operation will refresh the Cache reference using ZCacheable_getManager().
This implies the following behavior for the 3 affected methods:

ZCacheable_get -- invalidates its volatile reference and returns default, just like a normal cache miss
ZCacheable_set -- invalidates its volatile reference, this will incur another cache miss in the future at the time of the next ZCacheable_get()
ZCacheable_invalidate -- invalidates its volatile reference, returns a normal message saying the entry has been invalidated, (which is something of an understatement as the entire cache has evidently been destroyed <g>)

Risk factors, challenged assumptions, & API changes:

All in all the refactored code is almost totally backwards compatible, there are only a few minor API changes, in particular:

The Cache.py module variables ViewManagementScreensPermission, and ChangeCacheSettingsPermission have been changed to MANAGE_PERM and CHANGE_PERM respectively, because intollerably long variable names piss me off. Nowhere else in the entire Zope code base were these variables referenced, nor in any of the Cache products on Zope.org.

The managersExist() function was removed from the Cache.py module, it was only called in one spot from one another def in the same module so I just inlined it. Nowhere else in the entire Zope code base was this function referenced, nor in any of the Cache products on Zope.org.

Caches which depend on the greedy except nature of the old API will not behave as expected. I'd argue they were buggy anyway, but they can easily use the new CacheException base class to fix the issue. I'll include patches which do just this for RAMCacheManager.py. I examined the Cache products on Zope.org and found no behavior which relied upon the greedy nature of the old API.

There is some rather bizzare behavior which occurs when you've got nested cache managers, particularly of the same name. AFAICT this behavior was intentional, so I didn't change it, but if you add a cache manager with the same id as a manager above it in the containment hierarchy, documents which used to be assocated with the higher manager will be automatically placed into the new manager instead. Same story for deletion.

Appendix:
Cache products found on zope.org which I reference above were FSCacheManager and ZopeProxyCacheManager. There was a very old one called SimpleCache, but its no longer relevant. The listed bugs fixed are all security issues, but this bug isn't marked as security related because they are quite minor in the grand scheme of things. The refactored code has been tested extensively, I believe it to be sound.

[Jamie Heilman (jamie-audible)]

Uploaded: cachemanagers.diff

and here's a patch which accompanies the refactor, used to fit the current StandardCacheManagers to the refactored Cache.py exception API, use the base class's notion of permission values rather than hardcode them, and lastly remove all 3 bare except clauses from RAMCacheManager.py

[Jamie Heilman (jamie-audible)]

Uploaded: cmassoc.diff

This is a small diff to the Cacheable.ZCacheManager_associate DTMLFile lib/python/OFS/dtml/cmassoc.dtml, which cleans up the HTML and JavaScript removing several annoying bugs. It does not depend on either the new Cache.py or the previous patch in any way, but as its all related to the caching infrastucture I figured I'd include it here.

Also I made a silly typo below in my initial description; "...may be subclassed by Cache implementations, and only caches and logs exceptions if they are derived from that class." should read "...may be subclassed by Cache implementations, and only *catches* and logs exceptions if they are derived from that class." (I had caching on the brain, go figure.)

[Jamie Heilman (jamie-audible)]

fixed cachemanagers.diff below to not screw with the permissions, the old patch was dumb and clobbered "Change cache managers" with "Change cache settings". The name of the latter should really be "Change cache associations" to prevent confusion but its too late now I guess.

[Jamie Heilman (jamie-audible)]

Uploaded: Cache.py-2.3

cmassoc.diff (below) has been updated with more optimal javascript

Attached is a version of the refactored Cache.py updated for python 2.3

[Tres Seaver (tseaver)]

The drift in the current OFS/Cache.py compared to the version Jamie started refactoring from is pretty large. I'm afraid that without some help, we aren't ever going to be able to land the patch.

I think we would need to start by getting a chunk-by-chunk annotated diff between the uploaded version and the one it references (CVS revision 1.10, which I believe corresponds with SVN r22690).

We would then need to figure out how to port each logical change to the current branches. In the meanwhile, we have almost no test coverage for that module, which makes trying to land an extensive refactoring a pretty scary operation. So, I think we would have to start by writing a bunch of tests for the current branch, before even looking at Jamie's refactored version.

Technical debt sucks.

[Colin Watson (cjwatson)]

The zope2 project on Launchpad has been archived at the request of the Zope developers (see https://answers.launchpad.net/launchpad/+question/683589 and https://answers.launchpad.net/launchpad/+question/685285). If this bug is still relevant, please refile it at https://github.com/zopefoundation/zope2.