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]
