Intercepting private methods
avrecko
methods.
My code is(was) as follows
@Transactional // yes, Warp-persist:)
private void persistContact(Contact contact) {
daoProvider.get().makePersistent(contact);
}
public void addContact(Contact contact) {
try {
persistContact(contact);
reindexContact(contact);
} catch (RuntimeException e) {
throw e;
}
}
The code didn't work, Hibernate was complaining that it must be in
Active transaction, but hey I did put the @Transaction on the method.
After some quick debugging it turns out that the transaction
interceptor is missing! Ouch.
I changed the persistContact method to public and magic happens -
everything works as expected.
Is there a reason why private methods get left behind or did I missed
something? Other-wize Guice is quite a joy to work with.
Robbie Vanbrabant
http://google-guice.googlecode.com/svn/trunk/src/com/google/inject/ProxyFactory.java
From the cglib javadoc:
Due to the subclassing nature of the classes generated by Enhancer, the methods are guaranteed to be non-static, non-final, and non-private. Each method signature will only occur once, even if it occurs in multiple classes.
So for the reasoning you should talk to Bob, but I don't think that it's a bug.
For your code example it seems more logical to apply @Transactional to the entire operation instead of just the first part. Not that that answers your question, but at least for transactions you rarely need to apply them to private methods. Usually that's an indication that you need to refactor. But opinions may vary.
In any case, it would indeed be interesting to find out why it works the way it works.
Robbie
Dhanji R. Prasanna
invoke super.privateMethod() so there is no way for the cglib proxy to
invoke the proceeding joinpoint (without reflection).
More importantly, even if it did invoke the delegate by reflection
(not sure what it does), java does not perform dynamic dispatch on
private methods. So:
private class MySuper {
private void test() {
System.out.println("hello from MySuper");
}
}
private class MySub extends MySuper{
private void test() {
System.out.println("hello from MySub");
}
}
Invoking:
((MySuper)new MySub()).test(); //this dispatches statically!
...will print "hello from MySuper"
On the other hand changing the access modifier to protected, with the
same line of client code, will print "hello from MySub" (because
protected+ methods are dynamically dispatched).
Dhanji.
Robbie Vanbrabant
http://www.manning-sandbox.com/message.jspa?messageID=21112
Dhanji R. Prasanna
what Ramnivas (Spring AOP author) points out on a call() pointcut. But
on an execution() pointcut it can simply rewrite the method body
itself.
With build and load-time weaving technically anything is possible. By
comparison, Guice AOP (cglib) only use run-time weaving, which is why
we have limited instrumentation of [already] loaded classes.
avrecko
done my homework:) Found this paper on asm[1] fascinating stuff.
Before this discussion cglib was black box to me, now it is not so
black. Thanks again.
Alen
[1] http://download.forge.objectweb.org/asm/asm-guide.pdf
On 21 okt., 00:53, "Dhanji R. Prasanna" <dha...@gmail.com> wrote:
> For AspectJ it depends on the pointcut. The AspectJ compiler may do
> what Ramnivas (Spring AOP author) points out on a call() pointcut. But
> on an execution() pointcut it can simply rewrite the method body
> itself.
>
> With build and load-time weaving technically anything is possible. By
> comparison, Guice AOP (cglib) only use run-time weaving, which is why
> we have limited instrumentation of [already] loaded classes.
>
> Dhanji.
>
> On 10/21/07, Robbie Vanbrabant <robbie.vanbrab...@gmail.com> wrote:
>
> > Interesting problem. I always wondered how AspectJ does that. They generate
> > public wrappers, it seems:
> >http://www.manning-sandbox.com/message.jspa?messageID=21112
>
> > On 10/21/07, Dhanji R. Prasanna <dha...@gmail.com> wrote:
>
> > > That's the right answer, the bytecode verifier will balk if you try to
> > > invoke super.privateMethod() so there is no way for the cglib proxy to
> > > invoke the proceeding joinpoint (without reflection).
>
> > > More importantly, even if it did invoke the delegate by reflection
> > > (not sure what it does), java does not perform dynamic dispatch on
> > > private methods. So:
>
> > > private class MySuper {
> > > private void test() {
> > > System.out.println("hello from MySuper");
> > > }
> > > }
>
> > > private class MySub extends MySuper{
> > > private void test() {
> > > System.out.println("hello from MySub");
> > > }
> > > }
>
> > > Invoking:
>
> > > ((MySuper)new MySub()).test(); //this dispatches statically!
>
> > > ...will print "hello from MySuper"
>
> > > On the other hand changing the access modifier to protected, with the
> > > same line of client code, will print "hello from MySub" (because
> > > protected+ methods are dynamically dispatched).
>
> > > Dhanji.
>
> > > On 10/21/07, Robbie Vanbrabant <robbie.vanbrab...@gmail.com> wrote:
> > > > Guice seems to use Enhancer.getMethods to get the methods to intercept.
>
> >http://google-guice.googlecode.com/svn/trunk/src/com/google/inject/Pr...
>
> > > > From the cglib javadoc:
> > > > Due to the subclassing nature of the classes generated by Enhancer, the
> > > > methods are guaranteed to be non-static, non-final, and non-private.
> > Each
> > > > method signature will only occur once, even if it occurs in multiple
> > > > classes.
>
> > > > So for the reasoning you should talk to Bob, but I don't think that it's
> > a
> > > > bug.
> > > > For your code example it seems more logical to apply @Transactional to
> > the
> > > > entire operation instead of just the first part. Not that that answers
> > your
> > > > question, but at least for transactions you rarely need to apply them to
> > > > private methods. Usually that's an indication that you need to refactor.
> > But
> > > > opinions may vary.
>
> > > > In any case, it would indeed be interesting to find out why it works the
> > way
> > > > it works.
>
> > > > Robbie
>
Brian Pontarelli
Just a random thought. Wouldn't it be possible if the generated
sub-class was essentially a copy of the super-class with the weaving
added? This is heavy weight, but technically, wouldn't that work as well?
-bp
Bob Lee
Just a random thought. Wouldn't it be possible if the generated
sub-class was essentially a copy of the super-class with the weaving
added? This is heavy weight, but technically, wouldn't that work as well?
You wouldn't be able to use instances of the new class in place of the super class. :(
Bob
Brian Pontarelli
Why not? ;)
It still extends that class, but just duplicates everything inside it.
It doesn't actually ever use anything from its parent...
Class A
------------
+ foo() { call bar }
# bar() { call baz }
- baz() { whatever }
Class A' extends A
--------------------------------
+ foo() { pre; call bar; post }
# bar() { pre; call baz; post }
- baz() { pre; whatever; post }
It feels like I'm missing something, but I can't figure out what...
A a = new A'() // This works
a.foo() // This should work and call A'.foo
Am I missing something? Some issue with member variables?
-bp
Bob Lee wrote:
> On 10/22/07, *Brian Pontarelli* <br...@pontarelli.com
Bob Lee
You have some of the same problems here that you have creating separate proxies from concrete classes (like you see in Hibernate and Spring):
1) You duplicate internal state (ignoring the state in the parent class). The wasted memory probably isn't a big deal.
2) The parent classes can access private state in other instances. For example, I access fields directly from equals() methods all the time.
3) You can't have *any* final methods. With the current approach, you just can't intercept final methods. With this new approach, final methods would enable access to the parent class's state, etc.
Bob
Brian Pontarelli
Yep. It was #2 that I was missing. I had a feeling it was something with
member variables, but I couldn't recall what it was. So, essentially,
you can't handle A.method(A') or A'.method(A) for private fields.
Although, there are probably other hacks for the latter, but none I can
think of for the former. Yeah, that's a tough one....
-bp
Bob Lee wrote:
> Oh! I didn't realize you were still extending the class.
>
> You have some of the same problems here that you have creating
> separate proxies from concrete classes (like you see in Hibernate and
> Spring):
>
> 1) You duplicate internal state (ignoring the state in the parent
> class). The wasted memory probably isn't a big deal.
>
> 2) The parent classes can access private state in other instances. For
> example, I access fields directly from equals() methods all the time.
>
> 3) You can't have *any* final methods. With the current approach, you
> just can't intercept final methods. With this new approach, final
> methods would enable access to the parent class's state, etc.
>
> Bob
>
> On 10/22/07, *Brian Pontarelli* <br...@pontarelli.com
> <mailto:br...@pontarelli.com>> wrote:
>
> Why not? ;)
>
> It still extends that class, but just duplicates everything inside it.
> It doesn't actually ever use anything from its parent...
>
> Class A
> ------------
> + foo() { call bar }
> # bar() { call baz }
> - baz() { whatever }
>
>
> Class A' extends A
> --------------------------------
> + foo() { pre; call bar; post }
> # bar() { pre; call baz; post }
> - baz() { pre; whatever; post }
>
> It feels like I'm missing something, but I can't figure out what...
>
> A a = new A'() // This works
> a.foo() // This should work and call A'.foo
>
> Am I missing something? Some issue with member variables?
>
> -bp
>
>
>
> Bob Lee wrote:
> > On 10/22/07, *Brian Pontarelli* < br...@pontarelli.com
> <mailto:br...@pontarelli.com>
Bob Lee
Bob
Brian Pontarelli
That's the load time handling that was added in 5 right? I think it's in
java.lang.instrument. Anyone seen usages of it for AOP? Looked pretty
cool when they first started talking about it, but I've never played
around with it.
-bp
Bob Lee wrote:
> Yeah, you'd probably be better off using javax.instrument and
> rewriting the classes themselves.
>
> Bob
>
Dhanji R. Prasanna
no dynamic dispatch for private methods. The JVM does not inspect the
object at runtime to test whether it has polymorphic copy of the
private method (for obvious reasons). So there is no way to override a
private method -- a statement which is an oxymoron of sorts.
Brian Pontarelli
Right, which is why replacing the class byte code during class load time
would help to fix this issue. The weaving would be done by the
Instrumentation code using a direct call to Instrumentation or the
ClassFileTransformer interface. This would essentially be the same as
compile time weaving but occur at runtime. This would allow pretty much
all AOP including field and constructor aspects as well as aspects on
any methods, including statics, because you would be modifying the class
itself, rather than sub-classing. You wouldn't need to override the
method, you could just inject the byte code you needed into any class.
-bp
Dhanji R. Prasanna
Note that it's probably easier to use build-time weaving in this case.
javaagents produce notorious deployment slowdowns (as anyone who uses
aspectJ against large codebases will doubtless affirm ;).
Brian Pontarelli
me that agents would be the same speed or faster than run-time
sub-classing. What's the reason for the slow down? They are only
redefining classes at most one per classloader.... I'm assuming that
compile type is the fastest of the three though.
Bob Lee
I have very little experience in that department, but it would seem to
me that agents would be the same speed or faster than run-time
sub-classing. What's the reason for the slow down? They are only
redefining classes at most one per classloader.... I'm assuming that
compile type is the fastest of the three though.
I suspect the problem is with the performance of AspectJ itself. For every injection point, it has to search for matching pointcuts, something that probably doesn't exhibit constant performance, i.e. more code * more pointcuts = longer startup.
Bob
Dhanji R. Prasanna
To be fair to AspectJ, the likely reason is also the breadth of
additional joinpoints covered by each pointcut which is not a linear
increase in complexity.
Consider pointcut expression:
call( **.set*(..))
Has to ensure every *calling* class and line has to be instrumented.
Whereas aopalliance does not exhibit this facility.
Not to mention advising of more granular joinpoints such as cflows,
gets, sets, etc., which requires walking the bytecode of method bodies
and significantly more than is done at runtime by Guice/cglib.
..the elegance of the latter solution, notwithstanding ;)
Dhanji.
> Bob
>
>
>
> >
>
Brian Pontarelli
>> I suspect the problem is with the performance of AspectJ itself. For every
>> injection point, it has to search for matching pointcuts, something that
>> probably doesn't exhibit constant performance, i.e. more code * more
>> pointcuts = longer startup.
>>
that compile time weaving would not exhibit any performance implications
except a slower compile.
> To be fair to AspectJ, the likely reason is also the breadth of
> additional joinpoints covered by each pointcut which is not a linear
> increase in complexity.
>
> Consider pointcut expression:
>
> call( **.set*(..))
>
> Has to ensure every *calling* class and line has to be instrumented.
> Whereas aopalliance does not exhibit this facility.
>
> Not to mention advising of more granular joinpoints such as cflows,
> gets, sets, etc., which requires walking the bytecode of method bodies
> and significantly more than is done at runtime by Guice/cglib.
>
Gotcha. These pointcuts could definitely impact performance for sure and
should be linear as the number of classes and class size grows. It is
interesting stuff for sure. Although we did stray off-topic somewhat ;)
-bp