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? 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
