[jboss-as7-dev] Component upgrade of Javassist for AS7-5127...

[Stuart Douglas]

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/proxy/RuntimeSupport.java?u=-1&r1=591&r2=637
>>> 
>>> _______________________________________________
>>> jboss-as7-dev mailing list
>>> jboss-as7-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/jboss-as7-dev
>> 
> 
> 
> _______________________________________________
> jboss-as7-dev mailing list
> jboss-as7-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/jboss-as7-dev