commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Matt Benson <gudnabr...@gmail.com>
Subject Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
Date Wed, 08 Jun 2011 14:25:20 GMT
On Tue, Jun 7, 2011 at 8:01 PM, sebb <sebbaz@gmail.com> wrote:
> On 7 June 2011 21:49,  <mbenson@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-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
>
>

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message