Hi,
I'm not certain that I see the check I describe as particularly expensive, the
following code would be enough:
ClassReader cReader = new ClassReader(wovenClass.getBytes());
boolean mustBeWoven;
try {
Class<?> superClass = Class.forName(cReader.getSuperName().replace('/',
'.'), false,
wovenClass.getBundleWiring().getClassLoader());
mustBeWoven = WovenProxy.class.isAssignableFrom(superClass);
} catch (ClassNotFoundException e) {
String failureMessage =
NLS.MESSAGES.getMessage("fatal.weaving.failure", name);
//This is a failure that should stop the class loading!
LOGGER.error(failureMessage, e);
throw new WeavingException(failureMessage, e);
}
boolean weave = mustBeWoven || ....
The overhead of adding this code is limited to: creating a ClassReader from a
live byte array (very fast, nothing copied just a few constant pool entries
read) and reading a few entries from a byte array to build the super class name
String. The replace operation is also very fast, as it only needs to do some
char comparison on a String that is unlikely to be more than 100 chars long.
The only moderately expensive operation in the entire process is when we load
the superclass, however this will need to happen anyway when the subclass is
loaded, so it isn't wasted effort. As this code only needs to exist once in the
proxy weaving hook it also doesn't affect scaling. I see the safety benefit
that this code adds as a significant advantage at a very low performance cost.
Yes, the existing filtering approach exhibits the same problem that I've
described for the proposed one. I raised this same issue at the time, but
didn't have an immediate fix in mind. Now that I do have one I'd really like to
fix that filtering too.
As for the change you suggest to the ProxyWeavingController, it certainly gives
the implementation of the filter more information to play with, although I
think the method name should be something like "shouldWeaveBundle" to make sure
that people who don't read the JavaDoc aren't so surprised when they only get
called for the first class loaded by a given bundle and the result is cached
(rather than once for every WovenClass). Other than that, more information is
usually a good thing :)
Regards,
Tim Ward
-------------------
Apache Aries PMC member & Enterprise OSGi advocate
Enterprise OSGi in Action (http://www.manning.com/cummins)
-------------------
> Date: Mon, 6 Feb 2012 22:08:56 +0000
> Subject: Re: Customising the behaviour of the weaving proxy implementation
> From: [email protected]
> To: [email protected]
>
> Hi,
>
> I'm a little reluctant to do what you propose. In my scenario I can't get
> into the situation you describe and the overhead of validating the class
> hierarchy worries me for the scenario I have. I would also point out that
> the current baked in approach that disable certain classes or bundles
> suffers from the same issues.
>
> Instead I'm wondering about the following "major" modification to the
> interface:
>
> package org.apache.aries.proxy.weaving;
>
>
> import org.osgi.framework.Bundle;
>
> import org.osgi.framework.hooks.weaving.WovenClass;
>
>
> /**
>
> * Services of this interface are used by the ProxyManager's weaving
> implementation to
>
> * decide if a specific bundle should be subject to weaving.
>
> */
>
> public interface ProxyWeavingController
>
> {
>
> /**
>
> * Returns true if the bundle should be subject to proxy weaving. If it
> returns
>
> * false then the bundle will not be weaved. The result of this method is
> immutable
>
> * for a given bundle. That means repeated calls given the same bundle
> MUST
>
> * return the same response.
>
> *
>
> * @param clazz the class that is to be weaved
>
> * @return true if it should be woven, false otherwise.
>
> */
>
> public boolean shouldWeave(WovenClass clazz);
>
> }
>
> This would allow me to do what I need efficiently, it would also allow use
> cases I haven't considered. Then if someone returns false in the case you
> describe they can write the code and take the expense of ensuring the right
> consistency up the hierarchy.
>
> Thoughts?
> Alasdair
>
> On 6 February 2012 17:25, Timothy Ward <[email protected]> wrote:
>
> >
> > Hi,
> >
> > I like the principle behind this change, and think it probably is a good
> > way to go. It's lightweight, and results can be cached. There is one
> > negative point that affects this (and in fact almost all) filtering, wihch
> > is that the the weaving code is built around the fact that we weave
> > consistently throughout the class hierarchy up to a certain ancestor
> > (preferably java.lang.Object). I find this quite difficult to explain well
> > in words, so forgive the pretty trivial example.
> >
> >
> > We have three classes: Foo extends Bar, Bar extends Jimmy, and Jimmy
> > extends Object.
> >
> > When we are weaving Foo, the first thing we do is to load Bar. If Bar can
> > be woven then we will start by loading Jimmy. If Jimmy can be woven then we
> > start by loading Object (which cannot be woven).
> >
> > If all three classes can be woven then we add hooks into every class,
> > which is fine. If we can't weave Jimmy (which sometimes happens), then when
> > we weave Bar we can detect that Jimmy wasn't woven (because it doesn't
> > implement WovenProxy), and add overrides in Bar for any methods that it
> > inherits from Jimmy to allow us to weave in code. This means that Jimmy
> > can't be proxied (by the weaving code) but that Foo and Bar can.
> >
> > The problem is that this detection logic only spots the first break in the
> > weaving chain, and not subsequent breaks.
> >
> > If we were able to weave Foo and Jimmy, but not Bar, then the weaving code
> > would not know to override the methods that Foo inherits from Bar (Bar
> > implements WovenProxy because Jimmy does). Even worse, the weaving code
> > adds some synthetic methods to every class in the weaving hierarchy. These
> > will be missing from Bar, and use the implementations from Jimmy instead,
> > which will give the wrong answer. Also, because Bar will still implements
> > the WovenProxy interface the proxy bundle will think that Bar is proxyable,
> > even though it actually isn't.
> >
> >
> > I don't necessarily see this issue as a dealbreaker for the proposed
> > solution, but I do want people to be aware of the risks associated with
> > only weaving a subset of classes in a hierarchy.
> >
> > We could potentially prevent this situation by ignoring the bundle-wide
> > filter if the superclass of the class to be woven already implements
> > WovenProxy. This would mean that some classes from "unweavable" bundles
> > would be woven, but only if they were already part of a woven hierarchy
> > from some other bundle. It seems highly unlikely that this would affect
> > Alasdair's scenario (which I imagine is the common use case), but it also
> > prevents us from ever violating the assumptions made by the weaving code
> > about the continuity of the woven methods.
> >
> > All in all, a tentative +1. Alasdair, what do you think of a minor tweak
> > to override the filter in the case I describe above? I think it will be a
> > line or two change to your existing code, plus a very simple ClassVisitor.
> > We could even move some of the code out of the visit method of
> > AbstractWovenProxyAdapter and pass the super class as a java.lang.Class
> > constructor parameter to avoid duplicating effort. With this change I would
> > have no concerns about the solution at all.
> >
> > Regards
> >
> > Tim Ward
> > -------------------
> > Apache Aries PMC member & Enterprise OSGi advocate
> > Enterprise OSGi in Action (http://www.manning.com/cummins)
> > -------------------
> >
> >
> > > Date: Sat, 4 Feb 2012 23:43:38 +0000
> > > Subject: Customising the behaviour of the weaving proxy implementation
> > > From: [email protected]
> > > To: [email protected]
> > >
> > > Hi,
> > >
> > > I have been consuming the proxy bundle with the weaving support for a
> > while
> > > now, however it results in weaving classes that will never be proxies
> > ever.
> > > In fact I want to "weave" the classes that live in applications, but not
> > my
> > > runtime. I would like to be able to disable weaving for bundles in my
> > > runtime.
> > >
> > > I have prototyped a change locally which involves providing a new
> > service:
> > >
> > > package org.apache.aries.proxy.weaving;
> > >
> > >
> > > import org.osgi.framework.Bundle;
> > >
> > >
> > > /**
> > >
> > > * Services of this interface are used by the ProxyManager's weaving
> > > implementation to
> > >
> > > * decide if a specific bundle should be subject to weaving.
> > >
> > > */
> > >
> > > public interface ProxyWeavingController
> > >
> > > {
> > >
> > > /**
> > >
> > > * Returns true if the bundle should be subject to proxy weaving. If it
> > > returns
> > >
> > > * false then the bundle will not be weaved. The result of this method
> > is
> > > immutable
> > >
> > > * for a given bundle. That means repeated calls given the same bundle
> > > MUST
> > >
> > > * return the same response.
> > >
> > > *
> > >
> > > * @param b the bundle that is being weaved
> > >
> > > * @return true if it should be woven, false otherwise.
> > >
> > > */
> > >
> > > public boolean shouldWeave(Bundle b);
> > >
> > > }
> > >
> > > I've updated the proxy weaving support to call services and only if they
> > > all agree that the bundle should be woven will it be woven. I'd like to
> > > commit this to the codebase, but I wanted to get peoples thoughts before
> > I
> > > did.
> > >
> > > Thanks
> > > Alasdair
> > >
> > > --
> > > Alasdair Nottingham
> > > [email protected]
> >
> >
>
>
>
> --
> Alasdair Nottingham
> [email protected]
