Index: src/main/java/info/magnolia/module/cache/ehcache/EhCacheWrapper.java =================================================================== --- src/main/java/info/magnolia/module/cache/ehcache/EhCacheWrapper.java (revision 40482) +++ src/main/java/info/magnolia/module/cache/ehcache/EhCacheWrapper.java (working copy) @@ -41,6 +41,7 @@ import info.magnolia.module.cache.mbean.CacheMonitor; import net.sf.ehcache.Ehcache; import net.sf.ehcache.Element; +import net.sf.ehcache.constructs.blocking.BlockingCache; import net.sf.ehcache.constructs.blocking.LockTimeoutException; /** @@ -48,17 +49,28 @@ * @author gjoseph * @version $Revision: $ ($Author: $) */ -public class EhCacheWrapper implements Cache { +public class EhCacheWrapper implements info.magnolia.module.cache.BlockingCache { private static final Logger log = LoggerFactory.getLogger(EhCacheWrapper.class); - private final Ehcache ehcache; + private final BlockingCache ehcache; private String name; - public EhCacheWrapper(Ehcache ehcache, String name) { + public EhCacheWrapper(BlockingCache ehcache, String name) { this.ehcache = ehcache; this.name = name; } + public EhCacheWrapper(Ehcache ehcache, String name) { + this(castToBlockingCacheOrThrowException(ehcache), name); + } + + private static BlockingCache castToBlockingCacheOrThrowException(Ehcache ehcache) { + if(!(ehcache instanceof BlockingCache)){ + throw new RuntimeException("The current caching framework depends on the fact the a blocking cache is used."); + } + return (BlockingCache) ehcache; + } + public Object get(Object key) { final Element element = ehcache.get(key); try { @@ -101,6 +113,17 @@ ehcache.removeAll(); } + public void unlock(Object key) { + if(ehcache.getQuiet(key) == null) { + put(key, null); + remove(key); + } + } + + public int getBlockingTimeout() { + return ehcache.getTimeoutMillis(); + } + public Ehcache getWrappedEhcache() { return ehcache; } @@ -113,4 +136,6 @@ return ehcache.getSize(); } + + } Index: src/main/java/info/magnolia/module/cache/BlockingCache.java =================================================================== --- src/main/java/info/magnolia/module/cache/BlockingCache.java (revision 0) +++ src/main/java/info/magnolia/module/cache/BlockingCache.java (revision 0) @@ -0,0 +1,53 @@ +/** + * This file Copyright (c) 2010 Magnolia International + * Ltd. (http://www.magnolia-cms.com). All rights reserved. + * + * + * This file is dual-licensed under both the Magnolia + * Network Agreement and the GNU General Public License. + * You may elect to use one or the other of these licenses. + * + * This file is distributed in the hope that it will be + * useful, but AS-IS and WITHOUT ANY WARRANTY; without even the + * implied warranty of MERCHANTABILITY or FITNESS FOR A + * PARTICULAR PURPOSE, TITLE, or NONINFRINGEMENT. + * Redistribution, except as permitted by whichever of the GPL + * or MNA you select, is prohibited. + * + * 1. For the GPL license (GPL), you can redistribute and/or + * modify this file under the terms of the GNU General + * Public License, Version 3, as published by the Free Software + * Foundation. You should have received a copy of the GNU + * General Public License, Version 3 along with this program; + * if not, write to the Free Software Foundation, Inc., 51 + * Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. + * + * 2. For the Magnolia Network Agreement (MNA), this file + * and the accompanying materials are made available under the + * terms of the MNA which accompanies this distribution, and + * is available at http://www.magnolia-cms.com/mna.html + * + * Any modifications to this file must keep this entire header + * intact. + * + */ +package info.magnolia.module.cache; + +/** + * Marker interface to expose the fact that this cache will block further calls to + * {@link #get(Object)} until an entry is put into the cache. This is mainly use to avoid concurrent + * caching of the same resources. + */ +public interface BlockingCache extends Cache { + + /** + * Will unlock the key. + */ + void unlock(Object key); + + /** + * If the timeout is reached an exception is thrown to unlock the blocked thread (in + * milliseconds). This guarantees that not all threads are waiting on a non-available resource. + */ + int getBlockingTimeout(); +} Index: src/main/java/info/magnolia/module/cache/filter/CacheFilter.java =================================================================== --- src/main/java/info/magnolia/module/cache/filter/CacheFilter.java (revision 40482) +++ src/main/java/info/magnolia/module/cache/filter/CacheFilter.java (working copy) @@ -36,12 +36,15 @@ import info.magnolia.cms.core.AggregationState; import info.magnolia.cms.filters.OncePerRequestAbstractMgnlFilter; import info.magnolia.context.MgnlContext; +import info.magnolia.module.cache.BlockingCache; import info.magnolia.module.cache.Cache; import info.magnolia.module.cache.CacheConfiguration; +import info.magnolia.module.cache.CacheFactory; import info.magnolia.module.cache.CacheModuleLifecycleListener; import info.magnolia.module.cache.CacheModule; import info.magnolia.module.cache.CachePolicyExecutor; import info.magnolia.module.cache.CachePolicyResult; +import info.magnolia.module.cache.ehcache.EhCacheFactory; import info.magnolia.module.cache.mbean.CacheMonitor; import javax.servlet.FilterChain; @@ -50,6 +53,8 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import net.sf.ehcache.constructs.blocking.LockTimeoutException; + import java.io.IOException; /** @@ -69,6 +74,10 @@ private CacheConfiguration cacheConfig; private Cache cache; + // to provide warning log messages when we run into timeouts we have to save the timeout + private int blockingTimeout = -1; + + public String getCacheConfigurationName() { return cacheConfigurationName; } @@ -92,7 +101,11 @@ final CacheModule cacheModule = getModule(); this.cacheConfig = cacheModule.getConfiguration(cacheConfigurationName); - this.cache = cacheModule.getCacheFactory().getCache(cacheConfigurationName); + this.cache = cacheModule.getCacheFactory().getCache(cacheConfigurationName); + + if(cache instanceof BlockingCache){ + blockingTimeout = ((BlockingCache)cache).getBlockingTimeout(); + } if (cacheConfig == null || cache == null) { log.error("The " + getName() + " CacheFilter is not properly configured, either cacheConfig(" + cacheConfig + ") or cache(" + cache + ") is null. Check if " + cacheConfigurationName + " is a valid cache configuration name. Will disable temporarily."); @@ -107,7 +120,8 @@ } public void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws IOException, ServletException { - final AggregationState aggregationState = MgnlContext.getAggregationState(); + + final AggregationState aggregationState = MgnlContext.getAggregationState(); final CachePolicyResult cachePolicyResult = cacheConfig.getCachePolicy().shouldCache(cache, aggregationState, cacheConfig.getFlushPolicy()); log.debug("Cache policy result: {}", cachePolicyResult); @@ -119,20 +133,27 @@ if (executor == null) { throw new IllegalStateException("Unexpected cache policy result: " + cachePolicyResult); } - executor.processCacheRequest(request, response, chain, cache, cachePolicyResult); + + try{ + final long start = System.currentTimeMillis(); + executor.processCacheRequest(request, response, chain, cache, cachePolicyResult); + final long end = System.currentTimeMillis(); - // TODO if the cache blocks we will have to add this again. - /* - finally{ - Object key = cachePolicyResult.getCacheKey(); - if (!cachePolicyResult.getBehaviour().equals(CachePolicyResult.bypass) && ( - ((EhCacheWrapper)cache).getWrappedEhcache().getQuiet(key) == null)) { - log.warn("Cache nearly blocked for key: {}, removed entry", key); - cache.put(key, null); - cache.remove(key); + if(blockingTimeout != -1 && (end-start) >= blockingTimeout){ + log.warn("The following URL took longer than {} seconds to render. This might cause timout exceptions on other requests to the same URI. [url={}], [key={}]", new Object[]{blockingTimeout/1000, request.getRequestURL(), cachePolicyResult.getCacheKey()}); + } + } + catch(LockTimeoutException timeout){ + log.warn("The following URL was blocked for longer than {} seconds. [url={}], [key={}]", new Object[]{blockingTimeout/1000, request.getRequestURL(), cachePolicyResult.getCacheKey()} ); + throw timeout; + } + catch (Throwable th) { + if(cachePolicyResult.getBehaviour().equals(CachePolicyResult.store) && cache instanceof BlockingCache){ + log.error("A request started to cache but never put a cache entry into the blocking cache. This would block the cache key for ever. We are removing the cache entry manually. [url={}], [key={}]", new Object[]{request.getRequestURL(), cachePolicyResult.getCacheKey()}); + ((BlockingCache) cache).unlock(cachePolicyResult.getCacheKey()); } + throw new RuntimeException(th); } - */ } } Index: src/main/java/info/magnolia/module/cache/executor/Store.java =================================================================== --- src/main/java/info/magnolia/module/cache/executor/Store.java (revision 40482) +++ src/main/java/info/magnolia/module/cache/executor/Store.java (working copy) @@ -36,6 +36,7 @@ import info.magnolia.cms.core.Content; import info.magnolia.cms.util.RequestHeaderUtil; import info.magnolia.context.MgnlContext; +import info.magnolia.module.cache.BlockingCache; import info.magnolia.module.cache.Cache; import info.magnolia.module.cache.CacheModule; import info.magnolia.module.cache.CachePolicy; @@ -68,78 +69,67 @@ public void processCacheRequest(HttpServletRequest request, HttpServletResponse response, FilterChain chain, Cache cache, CachePolicyResult cachePolicyResult) throws IOException, ServletException { CachedEntry cachedEntry = null; - try { - // will write to both the response stream and an internal byte array for caching - final ByteArrayOutputStream cachingStream = new ByteArrayOutputStream(); - - final SimpleServletOutputStream out = new SimpleServletOutputStream(cachingStream); - final CacheResponseWrapper responseWrapper = new CacheResponseWrapper(response, out) { - public void flushBuffer() throws IOException { - // do nothing we will flush later. - } - }; - - // setting Last-Modified to when this resource was stored in the cache. This value might get overriden by further filters or servlets. - final long cacheStorageDate = System.currentTimeMillis(); - responseWrapper.setDateHeader("Last-Modified", cacheStorageDate); - chain.doFilter(request, responseWrapper); + // will write to both the response stream and an internal byte array for caching + final ByteArrayOutputStream cachingStream = new ByteArrayOutputStream(); - if ((responseWrapper.getStatus() != HttpServletResponse.SC_MOVED_TEMPORARILY) && (responseWrapper.getStatus() != HttpServletResponse.SC_NOT_MODIFIED) && !responseWrapper.isError()) { - //handle gzip headers (have to be written BEFORE committing the response - int vote = getCompressionVote(responseWrapper, ResponseContentTypeVoter.class); - final boolean acceptsGzipEncoding = RequestHeaderUtil.acceptsGzipEncoding(request) && vote == 0; - if (acceptsGzipEncoding) { - RequestHeaderUtil.addAndVerifyHeader(responseWrapper, "Content-Encoding", "gzip"); - RequestHeaderUtil.addAndVerifyHeader(responseWrapper, "Vary", "Accept-Encoding"); // needed for proxies - } + final SimpleServletOutputStream out = new SimpleServletOutputStream(cachingStream); + final CacheResponseWrapper responseWrapper = new CacheResponseWrapper(response, out) { + public void flushBuffer() throws IOException { + // do nothing we will flush later. } + }; - // change the status (if appropriate) before flushing the buffer. - if (!response.isCommitted() && !ifModifiedSince(request, cacheStorageDate)) { - responseWrapper.setStatus(HttpServletResponse.SC_NOT_MODIFIED); + // setting Last-Modified to when this resource was stored in the cache. This value might get overriden by further filters or servlets. + final long cacheStorageDate = System.currentTimeMillis(); + responseWrapper.setDateHeader("Last-Modified", cacheStorageDate); + chain.doFilter(request, responseWrapper); + if ((responseWrapper.getStatus() != HttpServletResponse.SC_MOVED_TEMPORARILY) && (responseWrapper.getStatus() != HttpServletResponse.SC_NOT_MODIFIED) && !responseWrapper.isError()) { + //handle gzip headers (have to be written BEFORE committing the response + int vote = getCompressionVote(responseWrapper, ResponseContentTypeVoter.class); + final boolean acceptsGzipEncoding = RequestHeaderUtil.acceptsGzipEncoding(request) && vote == 0; + if (acceptsGzipEncoding) { + RequestHeaderUtil.addAndVerifyHeader(responseWrapper, "Content-Encoding", "gzip"); + RequestHeaderUtil.addAndVerifyHeader(responseWrapper, "Vary", "Accept-Encoding"); // needed for proxies } + } - try { - response.flushBuffer(); - responseWrapper.flush(); + // change the status (if appropriate) before flushing the buffer. + if (!response.isCommitted() && !ifModifiedSince(request, cacheStorageDate)) { + responseWrapper.setStatus(HttpServletResponse.SC_NOT_MODIFIED); - } catch (IOException e) { - //TODO better handling ? - // ignore and don't cache, should be a ClientAbortException - return; - } + } - cachedEntry = makeCachedEntry(responseWrapper, cachingStream); - // cached page should have some body - if (cachedEntry != null && (cachedEntry instanceof CachedPage) && ((CachedPage) cachedEntry).getDefaultContent().length == 0) { - log.warn("Response body for {}:{} is empty.", String.valueOf(responseWrapper.getStatus()), cachePolicyResult.getCacheKey()); - } + try { + response.flushBuffer(); + responseWrapper.flush(); - // Cached page should be created only with 200 status and nothing else should go in - if ((cachedEntry instanceof CachedPage) && ((CachedPage) cachedEntry).getStatusCode() != HttpServletResponse.SC_OK) { - log.warn("Caching response {} for {}", String.valueOf(((CachedPage) cachedEntry).getStatusCode() ), cachePolicyResult.getCacheKey()); - } + } catch (IOException e) { + //TODO better handling ? + // ignore and don't cache, should be a ClientAbortException + return; + } + cachedEntry = makeCachedEntry(responseWrapper, cachingStream); + // cached page should have some body + if (cachedEntry != null && (cachedEntry instanceof CachedPage) && ((CachedPage) cachedEntry).getDefaultContent().length == 0) { + log.warn("Response body for {}:{} is empty.", String.valueOf(responseWrapper.getStatus()), cachePolicyResult.getCacheKey()); + } - } catch (Throwable t) { - log.error("Failed to process cache request : " + t.getMessage(), t); - } finally { - final Object key = cachePolicyResult.getCacheKey(); - // have to put cache entry no matter what even if it is null to release lock. - cache.put(key, cachedEntry); - if (cachedEntry == null ) { - cache.remove(cachePolicyResult.getCacheKey()); - } else { - cachePolicyResult.setCachedEntry(cachedEntry); - // let policy know the uuid in case it wants to do something with it - final Content content = MgnlContext.getAggregationState().getCurrentContent(); - if (content != null && content.isNodeType("mix:referenceable")) { - final String uuid = content.getUUID(); - String repo = content.getHierarchyManager().getName(); - getCachePolicy(cache).persistCacheKey(repo, uuid, key); - } - } + // Cached page should be created only with 200 status and nothing else should go in + if ((cachedEntry instanceof CachedPage) && ((CachedPage) cachedEntry).getStatusCode() != HttpServletResponse.SC_OK) { + log.warn("Caching response {} for {}", String.valueOf(((CachedPage) cachedEntry).getStatusCode() ), cachePolicyResult.getCacheKey()); + } + + final Object key = cachePolicyResult.getCacheKey(); + cache.put(key, cachedEntry); + cachePolicyResult.setCachedEntry(cachedEntry); + // let policy know the uuid in case it wants to do something with it + final Content content = MgnlContext.getAggregationState().getCurrentContent(); + if (content != null && content.isNodeType("mix:referenceable")) { + final String uuid = content.getUUID(); + String repo = content.getHierarchyManager().getName(); + getCachePolicy(cache).persistCacheKey(repo, uuid, key); } } Index: src/main/java/info/magnolia/module/cache/cachepolicy/Default.java =================================================================== --- src/main/java/info/magnolia/module/cache/cachepolicy/Default.java (revision 40482) +++ src/main/java/info/magnolia/module/cache/cachepolicy/Default.java (working copy) @@ -96,7 +96,7 @@ final Object cachedEntry = cache.get(key); // also assume that if the value can't be retrieved for given key, underlying cache is not a pig and will throw exception at some point releasing the thread and propagating the error to the user - if (cachedEntry != null) { + if (cachedEntry != null) { return new CachePolicyResult(CachePolicyResult.useCache, key, cachedEntry); } else { return new CachePolicyResult(CachePolicyResult.store, key, null);