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 <[email protected]> wrote: > On 13 September 2013 22:28, Gary Gregory <[email protected]> wrote: > > On Fri, Sep 13, 2013 at 4:27 PM, sebb <[email protected]> wrote: > > > >> On 13 September 2013 19:06, Gary Gregory <[email protected]> > 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 <[email protected]> > >> 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: [email protected] | [email protected] > >> > 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: [email protected] > >> For additional commands, e-mail: [email protected] > >> > >> > > > > > > -- > > E-Mail: [email protected] | [email protected] > > 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: [email protected] > For additional commands, e-mail: [email protected] > > -- E-Mail: [email protected] | [email protected] 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
