commons-user mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gary Gregory <garydgreg...@gmail.com>
Subject Re: [jxpath] race in getNodePointerFactories() ?
Date Fri, 13 Sep 2013 21:36:34 GMT
Hm... when I build trunk with Maven 3 "mvn clean test" and Java 7 on
Windows I get:

testLazyProperty(org.apache.commons.jxpath.ri.model.dynabeans.LazyDynaBeanTest)
Time elapsed: 0.004 sec  <<< ERROR!
org.apache.commons.jxpath.JXPathNotFoundException: No value for xpath:
/nosuch
        at
org.apache.commons.jxpath.ri.model.NodePointer.verify(NodePointer.java:937)
        at
org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.getValue(JXPathContextReferenceImpl.java:374)
        at
org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.getValue(JXPathContextReferenceImpl.java:315)
        at
org.apache.commons.jxpath.ri.model.dynabeans.LazyDynaBeanTest.testLazyProperty(LazyDynaBeanTest.java:34)

Gary


On Fri, Sep 13, 2013 at 5:28 PM, Gary Gregory <garydgregory@gmail.com>wrote:

> On Fri, Sep 13, 2013 at 4:27 PM, sebb <sebbaz@gmail.com> wrote:
>
>> On 13 September 2013 19:06, Gary Gregory <garydgregory@gmail.com> wrote:
>> > Hello David,
>> >
>> > Can you test with the 1.4-SNAPSHOT code from trunk? (You'll have to
>> check
>> > it out and build it).
>>
>> The method JXPathContextReferenceImpl.getNodePointerFactories() is not
>> synchronized in trunk either, and the static field is not volatile.
>> Furthermore, the method returns the array without copying it, so
>> callers can change entries in it.
>>
>> Also the field is created with a lock on the class, but is set to null
>> with a lock on the static feld nodeFactories.
>> And of course read with no lock at all.
>>
>> Not good; safe publication requires that reader and writer lock the
>> same object. And writers must use the same object for mutual
>> exclusion.
>>
>> The code is extremely fragile.
>>
>> > In trunk, NodePointer.java:80) is:
>> >
>> > pointer = new NullPointer(name, locale);
>> >
>> > which can't throw an NPE.
>>
>> 1.3 has the following code:
>>
>>         NodePointerFactory[] factories =
>>             JXPathContextReferenceImpl.getNodePointerFactories();
>>         for (int i = 0; i < factories.length; i++) { // <= line 80
>>
>> The for loop is at line 86 in trunk.
>>
>> > I have not looked at this code in a while, now that multi-core CPUs are
>> > common place I am not surprised to see issues like this.
>> >
>> > I am not even sure why the statics in JXPathContext are lazy loaded (at
>> > least they are volatile). It would simplify the code to just init the
>> > statics in-line.
>>
>> Mutable static fields are generally very bad for thread-safety.
>>
>
> I was not clear: The JXPathContext statics are only writing to by the
> getters for lazy-init. Why not init them in the decl and make them final?
>
> Gary
>
>>
>> > Thank you,
>> > Gary
>> >
>> >
>> > On Thu, Sep 12, 2013 at 10:11 PM, David Ferry <davidf@opencloud.com>
>> wrote:
>> >
>> >> Hi
>> >>
>> >> We're using Apache JXPath 1.3. We have multi-threaded code running
>> >> JXPathContext.newContext
>> >>
>> >> We're having an occasional NullPointerException coming out of
>> >> NodePointer.newNodePointer
>> >>
>> >> We've had a look at the code for this class, and wonder if
>> >> getNodePointerFactories is not synchronising correctly.
>> >>
>> >> Other operations that read or write the attribute nodeFactoryArray ...
>> >> synchronize on nodeFactories.
>> >>
>> >> But in getNodePointerFactories() ... it just returns the variable
>> without
>> >> locking.
>> >> This makes us think that there is no happens-before in the
>> >> getNodePointerFactories() method.
>> >>
>> >> I've put a snippet of the stack trace below.
>> >>
>> >> Apologies if this has come up before, I didn't manage to find it after
>> a
>> >> while searching.
>> >>
>> >> Regards,
>> >> -David
>> >>
>> >> java.lang.NullPointerException
>> >>         at
>> >>
>> org.apache.commons.jxpath.ri.model.NodePointer.newNodePointer(NodePointer.java:80)
>> >>         at
>> >>
>> org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.<init>(JXPathContextReferenceImpl.java:193)
>> >>         at
>> >>
>> org.apache.commons.jxpath.ri.JXPathContextReferenceImpl.<init>(JXPathContextReferenceImpl.java:167)
>> >>         at
>> >>
>> org.apache.commons.jxpath.ri.JXPathContextFactoryReferenceImpl.newContext(JXPathContextFactoryReferenceImpl.java:39)
>> >>         at
>> >>
>> org.apache.commons.jxpath.JXPathContext.newContext(JXPathContext.java:416)
>> >>
>> >
>> >
>> >
>> > --
>> > E-Mail: garydgregory@gmail.com | ggregory@apache.org
>> > Java Persistence with Hibernate, Second Edition<
>> http://www.manning.com/bauer3/>
>> > JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
>> > Spring Batch in Action <http://www.manning.com/templier/>
>> > Blog: http://garygregory.wordpress.com
>> > Home: http://garygregory.com/
>> > Tweet! http://twitter.com/GaryGregory
>>
>> ---------------------------------------------------------------------
>> To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
>> For additional commands, e-mail: user-help@commons.apache.org
>>
>>
>
>
> --
> E-Mail: garydgregory@gmail.com | ggregory@apache.org
> Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
> JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
> Spring Batch in Action <http://www.manning.com/templier/>
>
> Blog: http://garygregory.wordpress.com
> Home: http://garygregory.com/
> Tweet! http://twitter.com/GaryGregory
>



-- 
E-Mail: garydgregory@gmail.com | ggregory@apache.org
Java Persistence with Hibernate, Second Edition<http://www.manning.com/bauer3/>
JUnit in Action, Second Edition <http://www.manning.com/tahchiev/>
Spring Batch in Action <http://www.manning.com/templier/>
Blog: http://garygregory.wordpress.com
Home: http://garygregory.com/
Tweet! http://twitter.com/GaryGregory

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message