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 01:01:39 GMT
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.

> 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


Mime
View raw message