You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@commons.apache.org by sebb <se...@gmail.com> on 2011/06/08 03:01:39 UTC
Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
On 7 June 2011 21:49, <mb...@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
Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
Posted by James Carman <ja...@carmanconsulting.com>.
On Wed, Jun 8, 2011 at 11:11 AM, James Carman
<ja...@carmanconsulting.com> wrote:
>>
>
> Why are we going into a long, drawn-out discussion about this? We
> already know how to do a keyed cache (KeyedObjectPool anyone?). If
> you don't want to introduce a dependency, just borrow some code from
> Pool and be done with it.
>
Sorry, thought we were discussing the OGNL stuff here. I need to get
more sleep.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
Posted by James Carman <ja...@carmanconsulting.com>.
On Wed, Jun 8, 2011 at 11:06 AM, sebb <se...@gmail.com> wrote:
>
> 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.
>
Why are we going into a long, drawn-out discussion about this? We
already know how to do a keyed cache (KeyedObjectPool anyone?). If
you don't want to introduce a dependency, just borrow some code from
Pool and be done with it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org
Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
Posted by sebb <se...@gmail.com>.
On 8 June 2011 15:25, Matt Benson <gu...@gmail.com> wrote:
> On Tue, Jun 7, 2011 at 8:01 PM, sebb <se...@gmail.com> wrote:
>> On 7 June 2011 21:49, <mb...@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
Re: svn commit: r1133155 - /commons/proper/jxpath/trunk/src/java/org/apache/commons/jxpath/FunctionLibrary.java
Posted by Matt Benson <gu...@gmail.com>.
On Tue, Jun 7, 2011 at 8:01 PM, sebb <se...@gmail.com> wrote:
> On 7 June 2011 21:49, <mb...@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