commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From sebb <seb...@gmail.com>
Subject Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
Date Wed, 08 Jun 2011 15:06:46 GMT
On 8 June 2011 15:25, Matt Benson <gudnabrsam@gmail.com> wrote:
> 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,

Actually, that was not my point - fields written by one thread are not
necessarily published to other threads without synch.
Fields may be updated out of order or not at all.

However, I've just realised that there is a real update window:

1        if (byNamespace == null) {
2            synchronized (this) {
3                //read again
4                if (byNamespace == null) {
5                    byNamespace = new HashMap();

Thread A executes as far as completing line 5
If Thread B.then executes line 1, it may see the field as non-null,
but the map has not yet been populated.

Using volatile for byNamespace would not have helped here either -
same window can occur.

> 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
>
>

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


Mime
View raw message