2

Closed

OutputCache should use absolute expiration instead of sliding

description

DefaultCacheStorageProvider.Set() should read (where _clock is an injected IClock):
            _workContext.HttpContext.Cache.Add(
                key,
                cacheItem,
                null,
                _clock.UtcNow.AddSeconds(cacheItem.ValidFor),
                System.Web.Caching.Cache.NoSlidingExpiration,
                System.Web.Caching.CacheItemPriority.Normal,
                null);
instead of
            _workContext.HttpContext.Cache.Add(
                key,
                cacheItem,
                null,
                System.Web.Caching.Cache.NoAbsoluteExpiration,
                new TimeSpan(0, 0, cacheItem.ValidFor),
                System.Web.Caching.CacheItemPriority.Normal,
                null);
The fixed version caches the item for the amount of time configured (and also the entry is removed from the cache automatically when it expires without an access to it) by using absolute expiration.

The original version uses sliding expiration for the given amount of time. This means that the entry will expire if it is not accessed for the given amount of time. I.e. if expiration time is one minute but the page is accessed every 30 seconds it will never expire (until anything else clears it from the cache).

The above is also a fix for this issue.

Am I missing something?
Closed Oct 2, 2013 at 8:18 PM by Piedone

comments

BertrandLeRoy wrote Sep 26, 2013 at 11:23 PM

The cache entry should expire if:
  1. it's infrequently used.
  2. the associated contents changed.
So the current behavior seems ok to me, as long as 2 is implemented, which it is.

Piedone wrote Sep 27, 2013 at 8:48 AM

The issue is that OutputCache doesn't (and can't possibly) handle every scenario of changing content. While the content item change detection works well for the most straightforward scenarios (but still doesn't handle everything, like widgets) if the page's content doesn't come from a routed content item (or its container) it won't detect the change.

Using sliding expiration means that in case of pages accessed more frequently than the cache duration cache invalidation depends solely on the heuristic detecting content changes. If that doesn't handle every possible content change on the page (which is the case apart from the most simple scenarios) the cache will live on indefinitely.

AimOrchard wrote Sep 27, 2013 at 9:26 AM

"The cache entry should expire if:
it's infrequently used.
the associated contents changed."

Does that apply too for pages with a projection? (ie does it clear the cache if a projected item itself changes?)

Piedone wrote Sep 27, 2013 at 10:35 AM

It doesn't unless the Projection is also the container of the items.

Piedone wrote Sep 27, 2013 at 10:43 AM

I have to revise my previous statements regarding content item-driven invalidation: OutputCache seems to take any displayed item, also widgets, into account.

The other part still stands, this only helps if the whole content of a page is built up solely with content items (and not e.g. displaying anything from an external or any non-content item source).

BertrandLeRoy wrote Sep 27, 2013 at 11:03 PM

That there are problems with expiration mechanisms means that there is a problem with expiration mechanisms, not that we should use absolute expiration. If the page doesn't come from a routed content item, then the controller responsible for it should handle its own expiration mechanism. Or it can decide to use absolute expiration.
Also, the administrator can expire an entry manually when that's necessary.
Does that apply too for pages with a projection?
I'm not sure, and that's a good question. If it doesn't, then that needs to be fixed (as its own bug).

sebastienros wrote Sep 27, 2013 at 11:23 PM

I also think it should not use sliding expiration, for consistency with other providers, and also in case we miss a change. However I would have liked the sliding expiration to work as it would be more efficient, in a perfect world.

Piedone wrote Oct 2, 2013 at 8:18 PM

Fixed in changeset 61b614a0bbf719198e72d2713f19ea694365ab3b