So this means we can drop Marko' fix? On Sep 25, 2012, at 2:24, Stuart Douglas <[email protected]> wrote:
> This is the relevant AS7 fix: > > https://github.com/jbossas/jboss-as/pull/3112 > > Stuart > > Marko Lukša wrote: >> Hey >> >> I need someone to take a look at https://github.com/weld/core/pull/225 >> >> My initial change was to add bridge methods to the proxy (when proxying >> an interface), but later realized this could never work, because the >> interceptors expect a Method object that would have to point to a >> non-existing method (the method exists only on the proxy, but not on the >> interface). >> >> That's why the only solution was to make Beans.getInterceptableMethods() >> return the bridge methods also. The reason why you guys had problems >> with this in the past was the fact that MethodReference was missing the >> return type and thus treated two methods with the same name and >> parameters but different return types as equal, when they clearly >> aren't. This is why Weld was throwing the duplicate interceptor >> definition errors. >> >> Marko >> >> On 7.9.2012 17:43, Marko Lukša wrote: >>> Yes, when the proxy extends a class, everything is taken care of by >>> the bridge method in the superclass. But, when dealing with local EJB >>> interfaces, the proxy's superclass is Object, so there is no bridge >>> method at all. >>> >>> For EJBs, the stacktrace would look something like this: >>> StringFooImpl.doSth(String) >>> StringFooImpl.doSth(Object) >>> methodHandler.invoke("doSth(Object)") >>> StringFoo$Proxy.doSth(Object) <-- proxy extends Object, implements >>> StringFoo interface (no bridge method) >>> >>> For regular managed beans, the stacktrace would look like this: >>> StringFooImpl.doSth(String) >>> methodHandler.invoke("doSth(String)") >>> StringFooImpl$Proxy.doSth(String) >>> StringFooImpl$Proxy.doSth(Object) <-- proxy extends StringFooImpl >>> (and has bridge method) >>> >>> Note the different methods methodHandler is handling. >>> >>> Marko >>> >>> >>> On 7.9.2012 17:27, Stuart Douglas wrote: >>>> IMHO the correct way to deal with this is to simply make bridge >>>> methods delegate to super(), which will then result in the actual >>>> intercepted method being called. >>>> >>>> To be honest I thought we already did this, as we have had multiple >>>> related bugs in the past. Do you have a test case that I can look at? >>>> >>>> Stuart >>>> >>>>> Marko Lukša <mailto:[email protected]> >>>>> 7 September 2012 7:19 PM >>>>> Hey all. >>>>> >>>>> I've been working on https://issues.jboss.org/browse/WELD-1162 and >>>>> need your opinion. >>>>> >>>>> Say we have: >>>>> >>>>> public interface Foo<T> { >>>>> void doSomething(T t); >>>>> } >>>>> public interface StringFoo extends Foo<String> {} >>>>> public class StringFooImpl implements StringFoo {} >>>>> >>>>> and >>>>> >>>>> @Inject StringFoo stringFoo; >>>>> >>>>> The proxy created by Weld is a subclass of StringFooImpl and >>>>> therefore has two declared methods: >>>>> >>>>> void doSomething(Object o) { doSomething((String) o); } >>>>> void doSomething(String) {...} >>>>> >>>>> However, when StringFooImpl is a session bean, with StringFoo as its >>>>> local interface, the proxy is a subclass of Object and therefore the >>>>> proxy only has the following declared method: >>>>> >>>>> void doSomething(Object o); >>>>> >>>>> In both cases, when a client invokes stringFoo.doSomething("foo"), >>>>> the method doSomething(Object) is invoked. But there's a difference >>>>> in what happens next: >>>>> >>>>> * In the non-ejb version, the bridge method then immediately >>>>> invokes doSomething(String) and only then is the proxy's method >>>>> handler invoked. The handler is therefore dealing with the >>>>> method doSomething(*String*) >>>>> * in the EJB version, doSomething(Object) is not a bridge method, >>>>> and so the method handler is invoked directly and it (the >>>>> handler) is operating on doSomething(*Object*). >>>>> >>>>> In the second case, this ultimately means that Weld will check >>>>> whether doSomething(Object) is intercepted. It isn't, since >>>>> Beans.getInterceptableMethods() is ignoring bridge methods. The >>>>> interceptor will not be invoked. On the other hand, in the first >>>>> case, the interceptor _will_ be invoked, since Weld will be checking >>>>> whether doSomething(String) is intercepted. >>>>> >>>>> My initial solution was to make Beans.getInterceptableMethods() also >>>>> return bridge methods, but now I'm thinking the actual problem is in >>>>> the proxy itself. IMO, when creating a proxy based on an interface, >>>>> we should also generate bridge methods on the proxy (this should be >>>>> either done by Weld or by Javassist directly). These bridge methods >>>>> should be perfectly normal bridge methods and should not invoke the >>>>> method handler directly. They should simply invoke the non-bridge >>>>> method and the non-bridge method should then invoke the method handler. >>>>> >>>>> The java compiler can't add bridge methods directly to interfaces >>>>> which require them, so it adds them to all the classes implementing >>>>> the interface (StringFooImpl in our case). Since we are creating >>>>> StringFoo$Proxy, which is also a class implementing an interface >>>>> which requires bridge methods, we should add the bridge methods >>>>> to it - exactly as the java compiler would. >>>>> >>>>> This would solve the interceptor problem and possibly other similar >>>>> problems as well. >>>>> >>>>> What do you think? >>>>> >>>>> Marko >>>>> >>>>> >>>>> _______________________________________________ >>>>> weld-dev mailing list >>>>> [email protected] >>>>> https://lists.jboss.org/mailman/listinfo/weld-dev >>> >> >> _______________________________________________ >> weld-dev mailing list >> [email protected] >> https://lists.jboss.org/mailman/listinfo/weld-dev > _______________________________________________ > weld-dev mailing list > [email protected] > https://lists.jboss.org/mailman/listinfo/weld-dev _______________________________________________ weld-dev mailing list [email protected] https://lists.jboss.org/mailman/listinfo/weld-dev
