This content has been marked as final.
Show 24 replies
-
1. Re: MC upgrade breaks scoped aop tests in trunk
adrian.brock Nov 28, 2008 8:25 AM (in response to kabirkhan)"kabir.khan@jboss.com" wrote:
Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.
How did this work? I don't see anything in the previous GBF code that allowed you
to override the classloader (other than the metadata).However, now the underlying GenericBeanFactory will always use the loader of its KernelControllerContext to create the bean instance, so in the case outlined the wrong loader is used to create the bean.
This hasn't changed. All that changed is that this retrieval is now in a privileged block.
Asking the controller context essentially asks the ClassLoaderMetaData
if it has one (otherwise it uses the TCL),
which will be coming from whatever you setup in the deployer.
AbstractAopMetaDataDeployerprivate void deploy(FakeComponentUnit unit, BeanMetaData deployment) throws DeploymentException { // No explicit classloader, use the deployment's classloader if (deployment.getClassLoader() == null) { try { // Check the unit has a classloader unit.getClassLoader(); // TODO clone the metadata? deployment.setClassLoader(new DeploymentClassLoaderMetaData(unit)); // HERE } catch (Exception e) { log.debug("Unable to retrieve classloader for deployment: " + unit.getName() + " reason=" + e.toString()); } }
Rather than calling GBF directly from GBAF, if there is a pushed classloader I am trying to wrap GBF in a wrapper class that implements the same createBean() method, but with the option to specify the classloader. This gives me better results although I am still seeing some failures.
The ClassLoaderMetaData is also used to decide the classloader for other things,
e.g. string conversion to classes so I don't think being able to specify a classloader
on the GBF method is much help without rewriting all the metadata classes to also
allow the classloader to be overridden. -
2. Re: MC upgrade breaks scoped aop tests in trunk
adrian.brock Nov 28, 2008 8:59 AM (in response to kabirkhan)"adrian@jboss.org" wrote:
"kabir.khan@jboss.com" wrote:
Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.
How did this work? I don't see anything in the previous GBF code that allowed you
to override the classloader (other than the metadata).
NOTE: Changing the ClassLoaderMetaData on the GenericBeanFactory runtime
object won't help since that is just a copy of what the ControllerContext sees in
the BeanMetaData.
From GenericBeanFactoryMetaDataif (classLoader != null) { builder.setClassLoader(classLoader); // Used by the ControllerContext builder.addPropertyMetaData("classLoader", builder.createValue(classLoader)); // Used by the GBF }
-
3. Re: MC upgrade breaks scoped aop tests in trunk
kabirkhan Nov 28, 2008 8:59 AM (in response to kabirkhan)"adrian@jboss.org" wrote:
"kabir.khan@jboss.com" wrote:
Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.
How did this work? I don't see anything in the previous GBF code that allowed you
to override the classloader (other than the metadata).
The "pushed" metadata was the mechanism. For some reason the GBF.getClassLoader() always returned null, and I think the KCC loader was ignored previously. Anyway, whatever was put in by the metadata did work."adrian@jboss.org" wrote:
Asking the controller context essentially asks the ClassLoaderMetaData
if it has one (otherwise it uses the TCL),
which will be coming from whatever you setup in the deployer.
That is different from the ClassLoaderMetaData that is in GBF. Maybe not before I start messing around with things, but definitely after calling GBF.setClassLoader()."adrian@jboss.org" wrote:
AbstractAopMetaDataDeployerprivate void deploy(FakeComponentUnit unit, BeanMetaData deployment) throws DeploymentException { // No explicit classloader, use the deployment's classloader if (deployment.getClassLoader() == null) { try { // Check the unit has a classloader unit.getClassLoader(); // TODO clone the metadata? deployment.setClassLoader(new DeploymentClassLoaderMetaData(unit)); // HERE } catch (Exception e) { log.debug("Unable to retrieve classloader for deployment: " + unit.getName() + " reason=" + e.toString()); } }
Yes, that sets the correct classloader to use for the GenericBeanAspectFactory. But when needing to create an instance from a scoped classloader with no child delegation that also defines the class, that is the wrong loader (i.e. the parent). In all other cases this loader is fine."adrian@jboss.org" wrote:
Rather than calling GBF directly from GBAF, if there is a pushed classloader I am trying to wrap GBF in a wrapper class that implements the same createBean() method, but with the option to specify the classloader. This gives me better results although I am still seeing some failures.
The ClassLoaderMetaData is also used to decide the classloader for other things,
e.g. string conversion to classes so I don't think being able to specify a classloader
on the GBF method is much help without rewriting all the metadata classes to also
allow the classloader to be overridden. -
4. Re: MC upgrade breaks scoped aop tests in trunk
kabirkhan Nov 28, 2008 9:04 AM (in response to kabirkhan)What I am doing now and which makes all tests pass locally again is:
GenericBeanAspectFactory:protected Object doCreate(Advisor advisor, InstanceAdvisor instanceAdvisor, Joinpoint jp) { try { log.debug("Creating advice " + name); Object object = null; final ClassLoader loader = getCreatingClassLoader(advisor); if (loader == null) { object = factory.createBean(); } else { if (context == null) { throw new IllegalStateException("Attempting to create aspects in an undeployed factory"); } KernelConfigurator configurator = context.getKernel().getConfigurator(); ClassLoaderAwareGenericBeanFactory factoryWrapper = new ClassLoaderAwareGenericBeanFactory(configurator, loader, (GenericBeanFactory)factory); object = factoryWrapper.createBean(); } if (object instanceof XmlLoadable) { ((XmlLoadable)object).importXml(element); } configureInstance(object, advisor, instanceAdvisor, jp); return object; } catch (Throwable throwable) { throw new RuntimeException(throwable); } } private ClassLoader getCreatingClassLoader(Advisor advisor) { if (advisor == null) { return getLoader(); } return SecurityActions.getClassLoader(advisor.getClass()); }
The createBean() and invokeLifecycle() methods are modified copies of the GBF and AbstractBeanFactory equivalentspublic class ClassLoaderAwareGenericBeanFactory implements BeanFactory { ClassLoader loader; GenericBeanFactory delegate; KernelConfigurator configurator; public ClassLoaderAwareGenericBeanFactory(KernelConfigurator configurator, ClassLoader loader, GenericBeanFactory factory) { this.configurator = configurator; this.loader = loader; this.delegate = factory; } public Object createBean() throws Throwable { if (loader == null) { return delegate.createBean(); } else { BeanInfo info = null; if (delegate.getBean() != null) info = configurator.getBeanInfo(delegate.getBean(), loader, delegate.getAccessMode()); Joinpoint joinpoint = configurator.getConstructorJoinPoint(info, delegate.getConstructor(), null); Object result = joinpoint.dispatch(); if (info == null && result != null) info = configurator.getBeanInfo(result.getClass(),delegate.getAccessMode()); if (delegate.getProperties() != null && delegate.getProperties().size() > 0) { for (Map.Entry<String, ValueMetaData> entry : delegate.getProperties().entrySet()) { String property = entry.getKey(); ValueMetaData vmd = entry.getValue(); PropertyInfo pi = info.getProperty(property); pi.set(result, vmd.getValue(pi.getType(), loader)); } } invokeLifecycle("create", delegate.getCreate(), info, loader, result); invokeLifecycle("start", delegate.getStart(), info, loader, result); return result; } } protected void invokeLifecycle(String methodName, LifecycleMetaData lifecycle, BeanInfo info, ClassLoader cl, Object target) throws Throwable { if (lifecycle == null || lifecycle.isIgnored() == false) { String method = methodName; if (lifecycle != null && lifecycle.getMethodName() != null) method = lifecycle.getMethodName(); List<ParameterMetaData> parameters = null; if (lifecycle != null) parameters = lifecycle.getParameters(); MethodJoinpoint joinpoint; try { joinpoint = configurator.getMethodJoinPoint(info, cl, method, parameters, false, true); } catch (JoinpointException ignored) { return; } joinpoint.setTarget(target); joinpoint.dispatch(); } } }
-
5. Re: MC upgrade breaks scoped aop tests in trunk
adrian.brock Nov 28, 2008 9:14 AM (in response to kabirkhan)"kabir.khan@jboss.com" wrote:
"adrian@jboss.org" wrote:
"kabir.khan@jboss.com" wrote:
Some code in GenericBeanAspectFactory.doCreate() handled that scenario and pushed the correct loader to use into the underlying GenericBeanFactory.
How did this work? I don't see anything in the previous GBF code that allowed you
to override the classloader (other than the metadata).
The "pushed" metadata was the mechanism. For some reason the GBF.getClassLoader() always returned null,
I think this is the real change isn't it? It would depend upon how you
created/deployed the GBF's metadata. How old is the previous code I showed?
and I think the KCC loader was ignored previously. Anyway, whatever was put in by the metadata did work.
This is the code from 7 months ago before recent changes.
You can see it looks at the context first
http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas/projects/microcontainer/trunk/kernel/src/main/java/org/jboss/beans/metadata/plugins/factory/GenericBeanFactory.java?revision=77665&view=markup
But actually looking at the logic behind all this, I don't see why we need to look
at the context for classloader since the next step is to ask the
ClassLoaderMetaData anyway (or use the TCL absent that).private Object createBean(ClassLoader cl) throws Throwable { ClassLoader loader = cl; if (loader == null) loader = Configurator.getClassLoader(classLoader);
For everybody else, this will be same as asking the controller context
since the "classLoader" above is the same ClassLoaderMetaData.
But for you who wants to change the logic, it will do something different. -
6. Re: MC upgrade breaks scoped aop tests in trunk
adrian.brock Nov 28, 2008 9:19 AM (in response to kabirkhan)"kabir.khan@jboss.com" wrote:
The createBean() and invokeLifecycle() methods are modified copies of the GBF and AbstractBeanFactory equivalents
Duplicating code just means it won't get fixed the next time somebody finds a bug
or adds a new feature. -
7. Re: MC upgrade breaks scoped aop tests in trunk
kabirkhan Nov 28, 2008 9:52 AM (in response to kabirkhan)"adrian@jboss.org" wrote:
I think this is the real change isn't it? It would depend upon how you
created/deployed the GBF's metadata. How old is the previous code I showed?
The AbstractAopMetaDataDeployer code is current trunk and has been there for some months"adrian@jboss.org" wrote:
This is the code from 7 months ago before recent changes.
You can see it looks at the context first
http://viewvc.jboss.org/cgi-bin/viewvc.cgi/jbossas/projects/microcontainer/trunk/kernel/src/main/java/org/jboss/beans/metadata/plugins/factory/GenericBeanFactory.java?revision=77665&view=markup
Actually, GBF.classLoader is what is used if present. Not the context:public Object createBean() throws Throwable { ClassLoader cl = null; if (classLoader == null && context != null) { try { cl = context.getClassLoader(); } catch (Throwable t) { log.trace("Unable to retrieve classloader from " + context); } } if (cl == null) cl = Configurator.getClassLoader(classLoader);
"adrian@jboss.org" wrote:
But actually looking at the logic behind all this, I don't see why we need to look
at the context for classloader since the next step is to ask the
ClassLoaderMetaData anyway (or use the TCL absent that).private Object createBean(ClassLoader cl) throws Throwable { ClassLoader loader = cl; if (loader == null) loader = Configurator.getClassLoader(classLoader);
For everybody else, this will be same as asking the controller context
since the "classLoader" above is the same ClassLoaderMetaData.
But for you who wants to change the logic, it will do something different.
So what do you suggest? Putting things back to more like the way they were? I've got an idea for a test that I can add to aop-mc-int that can help test for future regressions once this is working again."adrian@jboss.org" wrote:
Duplicating code just means it won't get fixed the next time somebody finds a bug
or adds a new feature.
True, I was just trying to find a way out of my Friday surprise mess :-) -
8. Re: MC upgrade breaks scoped aop tests in trunk
kabirkhan Nov 28, 2008 12:15 PM (in response to kabirkhan)I have added a test to mc Branch_2_0 org.jboss.test.microcontainer.test.OverriddenClassLoaderTestCase. This will fail until this is fixed, but should help against regressions in this area in future.
-
9. Re: MC upgrade breaks scoped aop tests in trunk
dimitris Nov 28, 2008 1:37 PM (in response to kabirkhan)So who's fixing this / when? A new release of MC is needed?
-
10. Re: MC upgrade breaks scoped aop tests in trunk
alesj Nov 28, 2008 1:42 PM (in response to kabirkhan)"dimitris@jboss.org" wrote:
So who's fixing this / when? A new release of MC is needed?
I can do the release (if needed), but dunno who's fixing this. -
11. Re: MC upgrade breaks scoped aop tests in trunk
anil.saldhana Nov 28, 2008 3:23 PM (in response to kabirkhan)"alesj" wrote:
"dimitris@jboss.org" wrote:
So who's fixing this / when? A new release of MC is needed?
I can do the release (if needed), but dunno who's fixing this.
While you are at the mc release, can you put back the system property for the real urls in VFSClassLoaderPolicy? -
12. Re: MC upgrade breaks scoped aop tests in trunk
alesj Nov 28, 2008 3:40 PM (in response to kabirkhan)"anil.saldhana@jboss.com" wrote:
While you are at the mc release, can you put back the system property for the real urls in VFSClassLoaderPolicy?
MC != CL ;-)
Dimitris means Kernel part of MC.
We should re-name the MC parts, as MC of MC is confusing. -
13. Re: MC upgrade breaks scoped aop tests in trunk
kabirkhan Nov 29, 2008 7:41 AM (in response to kabirkhan)"alesj" wrote:
"dimitris@jboss.org" wrote:
So who's fixing this / when? A new release of MC is needed?
I can do the release (if needed), but dunno who's fixing this.
I think you or Adrian need to do it, I'm not 100% clear on what Adrian means, but I think GBF needs to use the loaders in the same way as before? -
14. Re: MC upgrade breaks scoped aop tests in trunk
alesj Nov 29, 2008 2:45 PM (in response to kabirkhan)"kabir.khan@jboss.com" wrote:
I think you or Adrian need to do it
Why us?
It's your feature. ;-)"kabir.khan@jboss.com" wrote:
I'm not 100% clear on what Adrian means
As much as I could follow, he's just trying to say you need
to prepare metadata properly and things should work.
Easy. :-)