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

[Stuart Douglas]

Sure, I will try and have a look at in the next week or so. It should be fairly easy as the other proxy code is pretty much already written, it just needs to be translated from class file writer to javassist.

Stuart 

On 12/07/2012, at 12:14 AM, Steve Ebersole wrote:

> 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/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
>> 
>> 
>> _______________________________________________
>> jboss-as7-dev mailing list
>> jboss-as7-dev at lists.jboss.org
>> https://lists.jboss.org/mailman/listinfo/jboss-as7-dev