11:27 p.m.
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/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