Sorry that was my fault, I was rushing. I have found a few minor issues
which have changed the way I want to do this. First of all I was going to
use the org.apache.aries.proxy.weaving package, but that exists in the impl
project and I don't want a clash so I've used
org.apache.aries.proxy.weavinghook instead.
Also putting an abstract class in the api bundle feels wrong given what it
is doing, so to keep the API bundle simple I've instead gone for this
following update. It amounts to the same thing, so I plan to commit ahead
of comments, I hope you don't mind.
The ProxyWeavingController now looks like this:
package org.apache.aries.proxy.weavinghook;
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.
*
* <p>If multiple ProxyWeavingController are registered all will be
consulted to decide
* whether to weave or not. As soon as one service says to weave a class
then
* it will be woven and following services may not be consulted.
* </p>
*/
public interface ProxyWeavingController
{
/**
* Returns true if the class should be subject to proxy weaving. If it
returns
* false then the class 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 classToWeave the class that is a candidate to be weaved.
* @param helper a helper calss to allow the implementation to make
intelligent weaving decisions.
* @return true if it should be woven, false otherwise.
*/
public boolean shouldWeave(WovenClass classToWeave, WeavingHelper helper);
}
the WeavingHelper looks like this:
package org.apache.aries.proxy.weavinghook;
import org.osgi.framework.hooks.weaving.WovenClass;
/**
* This provides helper methods to allow a ProxyWeavingController to make
* sensible decisions without needing to know how the ProxyManager has
implemented
* the weaving support.
*
* @noimplement
*/
public interface WeavingHelper
{
/**
* Tests to see if the provided class has been woven for proxying.
*
* @param c the class to test
* @return true if it is woven, false otherwise.
*/
public boolean isWoven(Class<?> c);
/**
* Tests to see if the super class of the provided WovenClass has
* been woven to support proxying.
*
* @param wovenClass the class whose parent should be tested.
* @return true if it is woven, false otherwise.
*/
public boolean isSuperClassWoven(WovenClass wovenClass);
}
On 7 February 2012 21:46, Timothy Ward <[email protected]> wrote:
>
>
> > Date: Tue, 7 Feb 2012 14:19:14 +0000
> > Subject: Re: Customising the behaviour of the weaving proxy
> implementation
> > From: [email protected]
> > To: [email protected]
> >
> > Hi,
> >
> > In my updated proposal the method would better be called shouldWeaveClass
> > given I updated it to take a WovenClass rather than a Bundle.
>
> In which case I was confused by the JavaDoc, which looked like it was
> still talking about immutable results per bundle
>
> >
> > The reason I am concerned about the overhead is because I know in my
> > scenario the computation will always return false. That means I will
> take a
> > hit on every class loader and in my environment that is a lot of classes.
> > Performance is really important and I want to remove as much extra
> > processing as is required.
> >
> > I think writing a ProxyWeavingController is an advanced capability and
> > people need to do it correctly, if someone gets it wrong that isn't our
> > issue (after all people can cause huge issues if they badly write the
> > plugins to application install, or blueprint). To help people get this
> > right we could easily provide an abstract base class which performs the
> > validation for people who need it, but for those who don't they can get
> the
> > best performance possible in their scenario.
>
> If the controller is going to be called per-class rather than per-bundle
> then it can be made to work this way. I think I would like there to be an
> abstract base implementation that people can extend. I'm all for good
> performance, but I'd like to blunt some of the sharp edges where we can,
> and this is a nasty one.
>
> >
> > The scenario I have in mind is one where I want weaving for bundles in
> .eba
> > applications, but not for bundles installed by a different provisioner.
> > Given the way our .eba isolation model works that means I just need a
> > weaving hook that returns true for bundles installed via the application
> > module, and false otherwise.
> >
> > Alasdair
> >
> > On 7 February 2012 10:26, Timothy Ward <[email protected]> wrote:
> >
> > >
> > > 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]
> > >
> > >
> >
> >
> >
> > --
> > Alasdair Nottingham
> > [email protected]
>
>
--
Alasdair Nottingham
[email protected]
