On Tue, Jun 7, 2011 at 8:01 PM, sebb <seb...@gmail.com> wrote: > 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. >
You are of course correct that the map could have been re-nulled before hitting the return statement, and that synchronizing the entire method is a much simpler way to handle this whole thing. I will take your advice. Thanks, Seb. Matt >> 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 > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
