Stuart, would you be willing to help us look at "dump[ing] the javassist proxy factory, and copy the jboss-invocation proxy factories into the hibernate code base"? On Tue 10 Jul 2012 08:28:17 PM CDT, Stuart Douglas wrote: > > Just had some time to look into this. > > The javassist change is basically bogus, as it introduces the possibility that a handler may get null as one of the methods: > > if (methods[index] == null) { > Method m1 = thisMethod == null ? null findMethod(self, thisMethod, desc); > Method m0 = findSuperMethod(self, superMethod, desc); > synchronized (methods) { > if (methods[index] == null) { > methods[index + 1] = m1; > methods[index] = m0; > } > } > > According to the JVM memory model the > > if(methods[index] == null) > > can return false, while methods[index +1] is still null. Even though methods[index + 1] is assigned first. This is because the JVM is allowed to (and does) re-order writes, so another thread could see methods[index] written before methods[index + 1]. > > In the AS7 proxies I store the Methods in static final fields that are loaded in a static constructor, so we so not not need anything like this. > > It would probably not be to hard to change javasisst proxies to use a similar method, but I am not 100% sure that there would be no client visible changes. Perhaps it would be easiest to dump the javassist proxy factory, and copy the jboss-invocation proxy factories into the hibernate code base (and change them to use javassist rather than classfilewriter). > > Stuart > > On 11/07/2012, at 1:52 AM, Scott Marlow wrote: > >> On 07/06/2012 10:40 AM, Kabir Khan wrote: >>> >>> On 6 Jul 2012, at 13:57, Scott Marlow wrote: >>> >>>> We aren't using the latest Javassist on AS7 master. Any arguments >>>> against switching to the latest/shiniest 3.1.7.0-GA javassist? That has >>>> a fix for AS7-5127. >>>> >>>> Currently, the following AS7 modules have dependencies on Javassist: >>>> >>>> org.jboss.ws.native.jbossws-native-core >>>> org.jboss.weld.core >>>> org.jboss.as.weld >>>> org.scannotation.scannotation >>>> org.hibernate (envers also) >>>> >>>> Also, could someone (familiar with javassist) take a look at the code >>>> change that AS7-5127 wants brought into AS7? In the updated method, >>>> parameter "java.lang.reflect.Method[] methods" is read outside of the >>>> synchronized lock. >>> I believe the check on line 53 needs to happen in a synchronized block as well to make sure it gets the latest, so rather than >>> if (methods[index] == null) { >>> Method m1 = thisMethod == null ? null >>> : findMethod(self, thisMethod, desc); >>> Method m0 = findSuperMethod(self, superMethod, desc); >>> synchronized (methods) { >>> if (methods[index] == null) { >>> methods[index + 1] = m1; >>> methods[index] = m0; >>> } >>> } >>> } >>> >>> Something along the lines of this would be more correct >>> >>> Method existing = null; >>> synchronized (methods) { >>> Method existing = methods[index]; >>> } >>> if (existing == null) { >>> Method m1 = thisMethod == null ? null >>> : findMethod(self, thisMethod, desc); >>> Method m0 = findSuperMethod(self, superMethod, desc); >>> synchronized (methods) { >>> if (methods[index] == null) { >>> methods[index + 1] = m1; >>> methods[index] = m0; >>> } >>> } >>> } >>> >>> >> >> That looks better but probably not that much more concurrent then the >> original code that performed both the read/write in the same >> synchronization block. >> >> Does anyone know if the array is longer than two elements (just curious >> if it contains just two entries or multiple pairs?) >> >>> >>>> >>>> code change is here >>>> https://source.jboss.org/browse/Javassist/trunk/src/main/javassist/util/p... >>>> >>>> _______________________________________________ >>>> jboss-as7-dev mailing list >>>> jboss-as7-dev(a)lists.jboss.org >>>> https://lists.jboss.org/mailman/listinfo/jboss-as7-dev >>> >> >> >> _______________________________________________ >> jboss-as7-dev mailing list >> jboss-as7-dev(a)lists.jboss.org >> https://lists.jboss.org/mailman/listinfo/jboss-as7-dev > > > _______________________________________________ > jboss-as7-dev mailing list > jboss-as7-dev(a)lists.jboss.org > https://lists.jboss.org/mailman/listinfo/jboss-as7-dev