On 7 June 2011 21:49, <mben...@apache.org> wrote: > Author: mbenson > Date: Tue Jun 7 20:49:04 2011 > New Revision: 1133155 > > URL: http://svn.apache.org/viewvc?rev=1133155&view=rev > Log: > [JXPATH-141] FunctionLibrary Multithreading issue
I don't think this is fully thread-safe. Although the byNamespace Map is created under a lock, another thread may bypass the synch. block. For non-final fields to be published correctly, both the writer and reader threads need to use the same lock. I suggest removing the double-check on the lock, i.e. just make the functionCache() method synchronised. If the map has already been created, the code path is very short. > Modified: > > commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java > > Modified: > commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java > URL: > http://svn.apache.org/viewvc/commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java?rev=1133155&r1=1133154&r2=1133155&view=diff > ============================================================================== > --- > commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java > (original) > +++ > commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java > Tue Jun 7 20:49:04 2011 > @@ -20,6 +20,7 @@ import java.util.ArrayList; > import java.util.HashMap; > import java.util.Iterator; > import java.util.List; > +import java.util.Map; > import java.util.Set; > > /** > @@ -32,8 +33,8 @@ import java.util.Set; > * @version $Revision$ $Date$ > */ > public class FunctionLibrary implements Functions { > - private List allFunctions = new ArrayList(); > - private HashMap byNamespace = null; > + private final List allFunctions = new ArrayList(); > + private Map byNamespace; > > /** > * Add functions to the library > @@ -59,10 +60,7 @@ public class FunctionLibrary implements > * @return Set<String> > */ > public Set getUsedNamespaces() { > - if (byNamespace == null) { > - prepareCache(); > - } > - return byNamespace.keySet(); > + return functionCache().keySet(); > } > > /** > @@ -75,10 +73,7 @@ public class FunctionLibrary implements > */ > public Function getFunction(String namespace, String name, > Object[] parameters) { > - if (byNamespace == null) { > - prepareCache(); > - } > - Object candidates = byNamespace.get(namespace); > + Object candidates = functionCache().get(namespace); > if (candidates instanceof Functions) { > return ((Functions) candidates).getFunction( > namespace, > @@ -105,28 +100,34 @@ public class FunctionLibrary implements > /** > * Prepare the cache. > */ > - private void prepareCache() { > - byNamespace = new HashMap(); > - int count = allFunctions.size(); > - for (int i = 0; i < count; i++) { > - Functions funcs = (Functions) allFunctions.get(i); > - Set namespaces = funcs.getUsedNamespaces(); > - for (Iterator it = namespaces.iterator(); it.hasNext();) { > - String ns = (String) it.next(); > - Object candidates = byNamespace.get(ns); > - if (candidates == null) { > - byNamespace.put(ns, funcs); > - } > - else if (candidates instanceof Functions) { > - List lst = new ArrayList(); > - lst.add(candidates); > - lst.add(funcs); > - byNamespace.put(ns, lst); > - } > - else { > - ((List) candidates).add(funcs); > + private Map functionCache() { > + if (byNamespace == null) { > + synchronized (this) { > + //read again > + if (byNamespace == null) { > + byNamespace = new HashMap(); > + int count = allFunctions.size(); > + for (int i = 0; i < count; i++) { > + Functions funcs = (Functions) allFunctions.get(i); > + Set namespaces = funcs.getUsedNamespaces(); > + for (Iterator it = namespaces.iterator(); > it.hasNext();) { > + String ns = (String) it.next(); > + Object candidates = byNamespace.get(ns); > + if (candidates == null) { > + byNamespace.put(ns, funcs); > + } else if (candidates instanceof Functions) { > + List lst = new ArrayList(); > + lst.add(candidates); > + lst.add(funcs); > + byNamespace.put(ns, lst); > + } else { > + ((List) candidates).add(funcs); > + } > + } > + } > } > } > } > + return byNamespace; > } > } > > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
