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:28:08 GMT
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

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