2:57 p.m.
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.
code change is here
https://source.jboss.org/browse/Javassist/trunk/src/main/javassist/util/p...
4:40 p.m.
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;
}
}
}
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
5:52 p.m.
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
3:28 a.m.
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
4374
days inactive
4379
days old
3 comments
3 participants
participants (3)
-
Kabir Khan -
Scott Marlow -
Stuart Douglas