Mailing-List: contact user-help@commons.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Commons Users List" <user@commons.apache.org>
Received-SPF: pass (nike.apache.org: domain of garydgregory@gmail.com
 designates 209.85.214.44 as permitted sender)
MIME-Version: 1.0
In-Reply-To: 
 <CAOGo0VYyZJRFizLMhiCoeO-gb-4+Q3xFsq1Y-z_W3nSWYYEa5w@mail.gmail.com>
References: 
 <CALg5huh0dZ1nRQG00ey7nkBPgRcjJ+efOOn-4DLE6hRMkyY20Q@mail.gmail.com>
	<CACZkXPzh_Gf2cqNnDUgMSJSs43Z1Qy321QF3aSNdxcXbkA9nbg@mail.gmail.com>
	<CAOGo0VZMQg5buWrg00JEw3+1t16LdJwbCa1BC6pYQo0OcwcZHQ@mail.gmail.com>
	<CACZkXPzLuz4Qq1ZLNkT7Tjauiwb1AXZYzSw0jFeZGydJdf16_w@mail.gmail.com>
	<CAOGo0VYyZJRFizLMhiCoeO-gb-4+Q3xFsq1Y-z_W3nSWYYEa5w@mail.gmail.com>
Date: Fri, 13 Sep 2013 17:57:52 -0400
Message-ID: 
 <CACZkXPw83WffEg5X8manCWfbmuwQfWQRakuX65m5-7yBHw2_Pw@mail.gmail.com>
Subject: Re: [jxpath] race in getNodePointerFactories() ?
From: Gary Gregory <garydgregory@gmail.com>
To: Commons Users List <user@commons.apache.org>
Content-Type: multipart/alternative; boundary=bcaec52c5e7340176204e64af203

--bcaec52c5e7340176204e64af203
Content-Type: text/plain; charset=UTF-8

In the case of JXPathContext, the statics are private, so the change would
be safe. I'm not going to do that now because I see a unit test failure (as
mentioned in a previous message).

Gary


On Fri, Sep 13, 2013 at 5:40 PM, sebb <sebbaz@gmail.com> wrote:

> On 13 September 2013 22:28, 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?
>
> I was referring to JXPathContextReferenceImpl which is where the NPE
> originated.
>
> But JXPathContext could do with some work as well.
> Could use the IODH idiom for the lazy-init.
>
> Unfortunately it has several protected mutable fields.
> These should have been private, especially since they seem to have
> public getters/setters.
> Fixing that will break compat, unless the class can be shown to be
> internal only.
>
> > 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
>
> ---------------------------------------------------------------------
> 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

--bcaec52c5e7340176204e64af203--