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 Sat, 14 Sep 2013 15:16:49 GMT
Check. Let's do 1.5 in trunk at least then.

Gary


On Sat, Sep 14, 2013 at 8:22 AM, sebb <sebbaz@gmail.com> wrote:

> On 14 September 2013 12:54, Gary Gregory <garydgregory@gmail.com> wrote:
> > Maybe we should set a Java requirement for Jxpath 1.4? I would just
> > use Java 6 as a min for now. Or why not 7?
>
> I think 1.5 would be a more sensible base.
>
> That gives big advantages over 1.4 and 1.3 without potentially
> excluding users stuck on 1.5.
>
> There is no point requiring our users to use 1.7 (or 1.6) unless the
> code *requires* it.
>
> 1.6 has some additional useful features (mainly Charset), but in
> comparison with 1.4 => 1.5 is not a huge advance.
>
> > Gary
> >
> > On Sep 14, 2013, at 5:17, sebb <sebbaz@gmail.com> wrote:
> >
> >> Tests OK for me as well using Java 1.5.
> >> [I no longer have 1.3 or 1.4 installed]
> >>
> >> BTW, the pom does not have any compiler source or target settings so
> >> defaults to 1.3 (which is why it uses JUnit 3.8.1).
> >>
> >> On 14 September 2013 03:04, Gary Gregory <garydgregory@gmail.com>
> wrote:
> >>> Weird, now it works.
> >>>
> >>> G
> >>>
> >>>
> >>> On Fri, Sep 13, 2013 at 5:36 PM, Gary Gregory <garydgregory@gmail.com
> >wrote:
> >>>
> >>>> 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
> >>>
> >>>
> >>>
> >>> --
> >>> 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
> >>
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: user-unsubscribe@commons.apache.org
> > For additional commands, e-mail: user-help@commons.apache.org
> >
>
> ---------------------------------------------------------------------
> 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