Michele Vivoda created JXPATH-181:
-------------------------------------
Summary: ValueUtils modifies a static HashMap field without
synchronization
Key: JXPATH-181
URL: https://issues.apache.org/jira/browse/JXPATH-181
Project: Commons JXPath
Issue Type: Bug
Reporter: Michele Vivoda
ValueUtils uses a HashMap to cache instances of DynamicPropertyHandler that
otherwise creates from a java.lang.Class object.
https://github.com/apache/commons-jxpath/blob/JXPATH_1_3_RC4/src/java/org/apache/commons/jxpath/util/ValueUtils.java#L44
https://github.com/apache/commons-jxpath/blob/JXPATH_1_3_RC4/src/java/org/apache/commons/jxpath/util/ValueUtils.java#L549
Since the getDynamicPropertyHandler method is static and modifies the HashMap,
thread safety is broken.
The easiest option would be to use Collections.SynchronizedMap(), introducing
more locking.
An other option is to remove cache: Class.newInstance() is not so heavy since
all internal implementations of DynamicPropertyHandler perform almost no
initialization in constructor. A side effect of removing cache could be
external implementations of DynamicPropertyHandler not expecting to be created
multiple times, but not sure if this is the case.
I did some tests all creating an instance from a almost-no-initialization class
object using Class.newInstance(): 2 with cache (HashMap and
Collections.synchronizedMap()) and one without cache.
First one thread, 50000x1000 loops
junit.perf.util.AccessorHashMap :259 (12.84%) 21.99% of max
junit.perf.util.AccessorSynchmap :1178 (58.4%) 100.0% of max
junit.perf.util.AccessorNoCache :580 (28.75%) 49.24% of max
then 1000 threads 50000 loops
junit.perf.util.AccessorHashMap :128 (3.82%) 4.51% of max
junit.perf.util.AccessorSynchmap :2840 (84.95%) 100.0% of max
junit.perf.util.AccessorNoCache :375 (11.21%) 13.2% of max
>From results I see no cache is not so bad also compared to HashMap, perhaps
>more important is that with 1000 threads Synchmap uses much more time than
>with one thread (240%), while the other 2 implementations use less; test
>machine has more than one processor.
So I propose to remove caching from ValueUtils
If one day DynamicPropertyHandler implementations will be registered in some
static way like using property files (and not with static methods like now) -
then may be will be possible to cache only those registered in properties in an
unmodifiable not synchronized map and just create the others
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
