Author: vgritsenko Date: Tue Apr 19 19:52:09 2005 New Revision: 162003 URL: http://svn.apache.org/viewcvs?view=rev&rev=162003 Log: fix bug: expressionValuesCache was not populated. remove unnecessary synchronization.
Modified: cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/modules/input/XMLFileModule.java Modified: cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/modules/input/XMLFileModule.java URL: http://svn.apache.org/viewcvs/cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/modules/input/XMLFileModule.java?view=diff&r1=162002&r2=162003 ============================================================================== --- cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/modules/input/XMLFileModule.java (original) +++ cocoon/branches/BRANCH_2_1_X/src/java/org/apache/cocoon/components/modules/input/XMLFileModule.java Tue Apr 19 19:52:09 2005 @@ -85,18 +85,24 @@ * @author <a href="mailto:[EMAIL PROTECTED]">Christian Haul</a> * @version $Id$ */ -public class XMLFileModule extends AbstractJXPathModule implements Composable, ThreadSafe { +public class XMLFileModule extends AbstractJXPathModule + implements Composable, ThreadSafe { /** Static (cocoon.xconf) configuration location, for error reporting */ String staticConfLocation; + /** Cached documents */ - Map documents = null; - /** Default value for reloadability of sources */ + Map documents; + + /** Default value for reloadability of sources. Defaults to false. */ boolean reloadAll; - /** Default value for cachability of sources */ - boolean cacheAll = true; - /** Default value for cachability of xpath expressions. */ + + /** Default value for cachability of sources. Defaults to true. */ + boolean cacheAll; + + /** Default value for cachability of xpath expressions. Defaults to true. */ boolean cacheExpressions = true; + /** Default src */ String src; @@ -106,23 +112,30 @@ // // need two caches for Object and Object[] // + /** XPath expression cache for single attribute values. */ private Map expressionCache; + /** XPath expression cache for multiple attribute values. */ private Map expressionValuesCache; + /** * Takes care of (re-)loading and caching of sources. */ protected static class DocumentHelper { private boolean reloadable; private boolean cacheable; + /** Source location */ private String uri; + /** Source validity */ private SourceValidity validity; + /** Source content cached as DOM Document */ private Document document; + /** Remember who created us (and who's caching us) */ private XMLFileModule instance; @@ -188,8 +201,7 @@ * clear everything for each configured document. * (this is a quick fix, no time to do the whole) */ - this.instance.expressionCache.clear(); - this.instance.expressionValuesCache.clear(); + this.instance.flushCache(); } } } @@ -217,7 +229,6 @@ } - /** * Set the current <code>ComponentManager</code> instance used by this * <code>Composable</code>. @@ -227,16 +238,20 @@ this.resolver = (SourceResolver) manager.lookup(SourceResolver.ROLE); } - /* (non-Javadoc) - * @see org.apache.avalon.framework.activity.Disposable#dispose() - */ + /** + * Dispose this component + */ public void dispose() { super.dispose(); if (this.manager != null) { this.manager.release(this.resolver); - this.manager = null; this.resolver = null; + this.manager = null; } + + this.documents = null; + this.expressionCache = null; + this.expressionValuesCache = null; } /** @@ -258,21 +273,20 @@ */ public void configure(Configuration config) throws ConfigurationException { - this.staticConfLocation = config.getLocation(); super.configure(config); - this.reloadAll = config.getChild("reloadable").getValueAsBoolean(this.reloadAll); + this.staticConfLocation = config.getLocation(); + this.reloadAll = config.getChild("reloadable").getValueAsBoolean(false); + if (config.getChild("cachable", false) != null) { throw new ConfigurationException("Bzzt! Wrong spelling at " + config.getChild("cachable").getLocation() + ": please use 'cacheable', not 'cachable'"); } - this.cacheAll = config.getChild("cacheable").getValueAsBoolean(this.cacheAll); + this.cacheAll = config.getChild("cacheable").getValueAsBoolean(true); - Configuration[] files = config.getChildren("file"); - if (this.documents == null) { - this.documents = Collections.synchronizedMap(new HashMap()); - } + this.documents = Collections.synchronizedMap(new HashMap()); + Configuration[] files = config.getChildren("file"); for (int i = 0; i < files.length; i++) { boolean reload = files[i].getAttributeAsBoolean("reloadable", this.reloadAll); boolean cache = files[i].getAttributeAsBoolean("cacheable", this.cacheAll); @@ -284,30 +298,15 @@ } // init caches - this.expressionCache = Collections.synchronizedMap(new ReferenceMap(AbstractReferenceMap.SOFT, AbstractReferenceMap.SOFT)); - this.expressionValuesCache = - Collections.synchronizedMap(new ReferenceMap(AbstractReferenceMap.SOFT, AbstractReferenceMap.SOFT)); + this.expressionCache = new ReferenceMap(AbstractReferenceMap.SOFT, AbstractReferenceMap.SOFT); + this.expressionValuesCache = new ReferenceMap(AbstractReferenceMap.SOFT, AbstractReferenceMap.SOFT); } /** - * Get the DOM object that JXPath will operate on when evaluating - * attributes. This DOM is loaded from a Source, specified in the - * modeConf, or (if modeConf is null) from the - * [EMAIL PROTECTED] #configure(Configuration)}. - * @param modeConf The dynamic configuration for the current operation. May - * be <code>null</code>, in which case static (cocoon.xconf) configuration - * is used. Configuration is expected to have a <file> child node, and - * be of the form: - * <...> - * <file src="..." reloadable="true|false"/> - * </...> - * @param objectModel Object Model for the current module operation. + * Retrieve document helper */ - protected Object getContextObject(Configuration modeConf, Map objectModel) + private DocumentHelper getDocumentHelper(Configuration modeConf) throws ConfigurationException { - String src = this.src; - boolean reload = this.reloadAll; - boolean cache = this.cacheAll; boolean hasDynamicConf = false; // whether we have a <file src="..."> dynamic configuration Configuration fileConf = null; // the nested <file>, if any @@ -322,14 +321,11 @@ } } + String src = this.src; if (hasDynamicConf) { src = fileConf.getAttribute("src"); } - if (this.documents == null) { - this.documents = Collections.synchronizedMap(new HashMap()); - } - if (src == null) { throw new ConfigurationException( "No source specified" @@ -339,6 +335,8 @@ } if (!this.documents.containsKey(src)) { + boolean reload = this.reloadAll; + boolean cache = this.cacheAll; if (hasDynamicConf) { reload = fileConf.getAttributeAsBoolean("reloadable", reload); cache = fileConf.getAttributeAsBoolean("cacheable", cache); @@ -348,13 +346,34 @@ + fileConf.getLocation() + ": please use 'cacheable', not 'cachable'"); } - } + this.documents.put(src, new DocumentHelper(reload, cache, src, this)); } + return (DocumentHelper) this.documents.get(src); + } + + /** + * Get the DOM object that JXPath will operate on when evaluating + * attributes. This DOM is loaded from a Source, specified in the + * modeConf, or (if modeConf is null) from the + * [EMAIL PROTECTED] #configure(Configuration)}. + * @param modeConf The dynamic configuration for the current operation. May + * be <code>null</code>, in which case static (cocoon.xconf) configuration + * is used. Configuration is expected to have a <file> child node, and + * be of the form: + * <...> + * <file src="..." reloadable="true|false"/> + * </...> + * @param objectModel Object Model for the current module operation. + */ + protected Object getContextObject(Configuration modeConf, Map objectModel) + throws ConfigurationException { + DocumentHelper helper = getDocumentHelper(modeConf); + try { - return ((DocumentHelper) this.documents.get(src)).getDocument(this.manager, this.resolver, getLogger()); + return helper.getDocument(this.manager, this.resolver, getLogger()); } catch (Exception e) { if (getLogger().isDebugEnabled()) { getLogger().debug("Error using source " + src + "\n" + e.getMessage(), e); @@ -365,17 +384,16 @@ public Object getAttribute(String name, Configuration modeConf, Map objectModel) throws ConfigurationException { - return this.getAttribute(name, modeConf, objectModel, false); + return getAttribute(name, modeConf, objectModel, false); } public Object[] getAttributeValues(String name, Configuration modeConf, Map objectModel) throws ConfigurationException { - Object result = this.getAttribute(name, modeConf, objectModel, true); + Object result = getAttribute(name, modeConf, objectModel, true); return (result != null ? (Object[]) result : null); } - - public Object getAttribute(String name, Configuration modeConf, Map objectModel, boolean getValues) + private Object getAttribute(String name, Configuration modeConf, Map objectModel, boolean getValues) throws ConfigurationException { Object contextObj = getContextObject(modeConf, objectModel); if (modeConf != null) { @@ -386,11 +404,7 @@ Map cache = null; boolean hasBeenCached = false; if (this.cacheExpressions) { - if (getValues){ - cache = this.getValuesCache(contextObj); - } else { - cache = this.getCache(contextObj); - } + cache = getExpressionCache(getValues? this.expressionValuesCache: this.expressionCache, contextObj); hasBeenCached = cache.containsKey(name); if (hasBeenCached) { result = cache.get(name); @@ -422,32 +436,23 @@ return result; } - - private Map getCache(Object key) { - Map cache = (Map) this.expressionCache.get(key); - if (cache == null) { - cache = this.initSubExpressionCache(key); + protected void flushCache() { + synchronized(this.expressionCache) { + this.expressionCache.clear(); } - return cache; - } - - private Map getValuesCache(Object key) { - Map cache = (Map) this.expressionValuesCache.get(key); - if (cache == null) { - cache = this.initSubExpressionCache(key); + synchronized(this.expressionValuesCache) { + this.expressionValuesCache.clear(); } - return cache; } - private synchronized Map initSubExpressionCache(Object key) { - // check whether some other thread did all the work while we were waiting - Map cache = (Map) this.expressionCache.get(key); - if (cache == null) { - // need to use HashMap here b/c we need to be able to store nulls - // which are prohibited for the ReferenceMap - cache = Collections.synchronizedMap(new HashMap()); - this.expressionCache.put(key, cache); + private Map getExpressionCache(Map cache, Object key) { + synchronized (cache) { + Map map = (Map) cache.get(key); + if (map == null) { + map = Collections.synchronizedMap(new HashMap()); + cache.put(key, map); + } + return map; } - return cache; } }