You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2014/07/23 21:55:14 UTC
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
GitHub user neykov opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/90
OSGi class loading fixes
* Changes the way bundles on the classpath are exported to felix - from list of exported packages in system.packages.extra to building extension bundles for each bundle on the classpath. This avoids the case where the same class would be loaded once from the classpath and once from a bundle referenced by a catalog item. When the catalog item tries to install the bundle we see that there is already an existing version installed and use it instead.
* AggregateClassLoader doesn't relay to parent, using only the explicitly registered classloaders.
* Force maven-bundle-plugin to include \*.impl.\* and \*.internal.\* packages in the export-package list (referenced between brooklyn bundles)
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/neykov/incubator-brooklyn osgi-fixes
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/90.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #90
----
commit 6b3930415accb53e08345e323da1f2330defdcf4
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-07-23T15:21:20Z
writeToTempFile should follow the semantics of newTempFile
Os.*TempFile methods would be expected to behave in a similar manner.
Use newTempFile which will normalize prefix/suffix arguments.
commit 998080399b8c00f721ee771b44a1e6f48d925886
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-07-23T15:25:02Z
AggregateClassLoader shouldn't relay requests to the parent.
A normal class loading first goes through the parent class loaders and
if that fails then findClass will be called. In the aggregate case we have
already explicitly listed all classloaders we want to try. Don't try to load
classes from parent.
commit 812bf260c009498cb8f9f2577c88716e435a143b
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
Date: 2014-07-23T15:48:58Z
Resolve classloading issues related to OSGi.
The changes should make it impossible to have the same
class loaded from different classloaders resulting in linker
errors. For example a catalog item could load a class from
one of its linked bundles, while the same class would be available
on the app classpath where it was already loaded.
The solution is to make sure that a bundle is loaded exactly once.
While this is normally the case we were having issues with bundles
included in the boot classpath. We were exporting their packages
but the framework didn't know which bundle name:version they
belonged to, thus making it possible to load the same bundle from
external source.
Instead of including the exported packages of bundles on the
classpath to the system.packages.extra config, now for each bundle
on the classpath an extension bundle will be created and installed.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15418622
--- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
@@ -108,11 +108,22 @@ public void registerBundle(String bundleUrl) {
if (bundleNameVersion==null) {
bundleNameVersion = bundleUrlOrNameVersionString;
}
-
+
Maybe<Bundle> bundle = Osgis.getBundle(framework, bundleNameVersion);
if (bundle.isPresent()) {
- @SuppressWarnings("unchecked")
- Class<T> clazz = (Class<T>) bundle.get().loadClass(type);
+ Bundle b = bundle.get();
+ Class<T> clazz;
+ //Extension bundles don't support loadClass.
+ //Instead load from the app classpath.
+ if (Osgis.isExtensionBundle(b)) {
+ @SuppressWarnings("unchecked")
+ Class<T> c = (Class<T>)Class.forName(type);
--- End diff --
this will totally ignore the extension, right? do we need to try to load via the bundle being extended? or maybe, if extension bundles are installed, attempt a second pass which tries again to load via the non-bundle extensions?
(my understanding is that, if i had `bundle-A` and `extension-A1-for-bundle-A`, i'd have to install `bundle-A` first into the OSGi container, but an attempt to `loadClass("ClassInExtensionA1")` on `bundle-A` would fail until i've installed `extension-A1` -- but the code here doesn't attempt to reload with `bundle-A` after installing `extension-A1`. please correct me if i'm wrong!)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#issuecomment-50449669
Addressed comments.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15458743
--- Diff: core/src/main/java/brooklyn/util/osgi/Osgis.java ---
@@ -157,138 +160,214 @@ public static Framework newFrameworkStarted(String felixCacheDir, boolean clean,
Map<Object,Object> cfg = MutableMap.copyOf(extraStartupConfig);
if (clean) cfg.put(Constants.FRAMEWORK_STORAGE_CLEAN, "onFirstInit");
if (felixCacheDir!=null) cfg.put(Constants.FRAMEWORK_STORAGE, felixCacheDir);
- cfg.put(Constants.FRAMEWORK_SYSTEMPACKAGES_EXTRA, getBrooklynBootBundles());
FrameworkFactory factory = newFrameworkFactory();
-
+
+ long frameworkInitStart = System.currentTimeMillis();
Framework framework = factory.newFramework(cfg);
try {
framework.init();
- // nothing needs auto-loading, currently (and this needs a new dependency)
- // AutoProcessor.process(configProps, m_fwk.getBundleContext());
+ installBootBundles(framework);
framework.start();
} catch (Exception e) {
// framework bundle start exceptions are not interesting to caller...
throw Exceptions.propagate(e);
}
+ long frameworkInitEnd = System.currentTimeMillis();
+ double frameworkInitSeconds = ((double)frameworkInitEnd - frameworkInitStart) / 1000;
+ LOG.info("OSGi framework started in " + Strings.makeRealString(frameworkInitSeconds, -1, 2, -1) + " seconds.");
+
return framework;
}
- private static String getBrooklynBootBundles() {
+ private static void installBootBundles(Framework framework) {
Enumeration<URL> resources;
try {
- resources = Osgis.class.getClassLoader().getResources("META-INF/MANIFEST.MF");
+ resources = Osgis.class.getClassLoader().getResources(MANIFEST_PATH);
} catch (IOException e) {
throw Exceptions.propagate(e);
}
-
- Collection<String> exportPackages = new ArrayList<String>();
+ BundleContext bundleContext = framework.getBundleContext();
+ Map<String, Bundle> installedBundles = getInstalledBundles(bundleContext);
while(resources.hasMoreElements()) {
URL url = resources.nextElement();
- exportPackages.addAll(getBundleExportedPackages(url));
+ installExtensionBundle(bundleContext, url, installedBundles, getVersionedId(framework));
}
+ }
- Iterator<String> brooklynPackages = Iterators.filter(exportPackages.iterator(), new Predicate<String>() {
- @Override
- public boolean apply(String input) {
- return input.startsWith(BROOKLYN_PACKAGE_PREFIX);
- }
- });
-
- String bootBundles = Joiner.on(",").join(brooklynPackages);
- LOG.debug("Found the following boot OSGi packages: " + bootBundles);
- return bootBundles;
+ private static Map<String, Bundle> getInstalledBundles(BundleContext bundleContext) {
+ Map<String, Bundle> installedBundles = new HashMap<String, Bundle>();
+ Bundle[] bundles = bundleContext.getBundles();
+ for (Bundle b : bundles) {
+ installedBundles.put(getVersionedId(b), b);
+ }
+ return installedBundles;
}
- private static Collection<String> getBundleExportedPackages(URL manifestUrl) {
+ private static void installExtensionBundle(BundleContext bundleContext, URL manifestUrl, Map<String, Bundle> installedBundles, String frameworkVersionedId) {
+ //ignore http://felix.extensions:9/ system entry
+ if("felix.extensions".equals(manifestUrl.getHost())) return;
+
try {
- ManifestHelper helper = ManifestHelper.forManifest(manifestUrl);
- return helper.getExportedPackages();
+ Manifest manifest = readManifest(manifestUrl);
+ if (!isValidBundle(manifest)) return;
+ String versionedId = getVersionedId(manifest);
+ URL bundleUrl = ResourceUtils.getContainerUrl(manifestUrl, MANIFEST_PATH);
+
+ Bundle existingBundle = installedBundles.get(versionedId);
+ if (existingBundle != null) {
+ if (!bundleUrl.equals(existingBundle.getLocation()) &&
+ //the framework bundle is always pre-installed, don't display duplicate info
+ !versionedId.equals(frameworkVersionedId)) {
+ LOG.info("Ignoring duplicate " + versionedId + " from manifest " + manifestUrl + ", already loaded from " + existingBundle.getLocation());
+ }
+ return;
+ }
+
+ byte[] jar = buildExtensionBundle(manifest);
+ LOG.debug("Installing boot bundle " + bundleUrl);
+ //mark the bundle as extension so we can detect it later using the "system:" protocol
+ //(since we cannot access BundleImpl.isExtension)
+ Bundle newBundle = bundleContext.installBundle(EXTENSION_PROTOCOL + ":" + bundleUrl.toString(), new ByteArrayInputStream(jar));
+ installedBundles.put(versionedId, newBundle);
} catch (IOException e) {
LOG.warn("Unable to load manifest from " + manifestUrl + ", ignoring.", e);
} catch (BundleException e) {
LOG.warn("Unable to load manifest from " + manifestUrl + ", ignoring.", e);
}
- return Collections.emptyList();
}
+ private static Manifest readManifest(URL manifestUrl) throws IOException {
+ Manifest manifest;
+ InputStream in = null;
+ try {
+ in = manifestUrl.openStream();
+ manifest = new Manifest(in);
+ } finally {
+ if (in != null) {
+ try {in.close();}
+ catch (Exception e) {};
+ }
+ }
+ return manifest;
+ }
+
+ private static byte[] buildExtensionBundle(Manifest manifest) throws IOException {
+ Attributes atts = manifest.getMainAttributes();
+
+ //the following properties are invalid in extension bundles
+ atts.remove(new Attributes.Name(Constants.IMPORT_PACKAGE));
+ atts.remove(new Attributes.Name(Constants.REQUIRE_BUNDLE));
+ atts.remove(new Attributes.Name(Constants.BUNDLE_NATIVECODE));
+ atts.remove(new Attributes.Name(Constants.DYNAMICIMPORT_PACKAGE));
+ atts.remove(new Attributes.Name(Constants.BUNDLE_ACTIVATOR));
+
+ //mark as extension bundle
+ atts.putValue(Constants.FRAGMENT_HOST, "system.bundle; extension:=framework");
+
+ //create the jar containing the manifest
+ ByteArrayOutputStream jar = new ByteArrayOutputStream();
+ JarOutputStream out = new JarOutputStream(jar, manifest);
+ out.close();
+ return jar.toByteArray();
+ }
+
+ private static boolean isValidBundle(Manifest manifest) {
+ Attributes atts = manifest.getMainAttributes();
+ return atts.containsKey(new Attributes.Name(Constants.BUNDLE_MANIFESTVERSION));
+ }
+
+ private static String getVersionedId(Bundle b) {
+ return b.getSymbolicName() + ":" + b.getVersion();
+ }
+
+ private static String getVersionedId(Manifest manifest) {
+ Attributes atts = manifest.getMainAttributes();
+ return atts.getValue(Constants.BUNDLE_SYMBOLICNAME) + ":" +
+ atts.getValue(Constants.BUNDLE_VERSION);
+ }
/**
* Installs a bundle from the given URL, doing a check if already installed, and
* using the {@link ResourceUtils} loader for this project (brooklyn core)
*/
public static Bundle install(Framework framework, String url) throws BundleException {
- Bundle bundle = framework.getBundleContext().getBundle(url);
- if (bundle != null) {
- return bundle;
+ boolean isLocal = isLocalUrl(url);
+ String localUrl = url;
+ if (!isLocal) {
+ localUrl = cacheFile(url);
}
- // use our URL resolution so we get classpath items
- LOG.debug("Installing bundle into {} from url: {}", framework, url);
- InputStream stream = ResourceUtils.create(Osgis.class).getResourceFromUrl(url);
- return framework.getBundleContext().installBundle(url, stream);
- }
-
- public static class ManifestHelper {
--- End diff --
Definitely could be useful again. I did consider that but at the time decided that we could fetch it from history.
You are right that it's harder to remember where to look for deleted code, I will keep it along with a comment that it is not used.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15458083
--- Diff: core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java ---
@@ -139,15 +138,16 @@ public InternalEntityFactory(ManagementContextInternal managementContext, Entity
}
builder.addAll(spec.getAdditionalInterfaces());
Set<Class<?>> interfaces = builder.build();
-
- // TODO OSGi strangeness! The classloader obtained from the type should be enough.
- // If an OSGi class loader, it should delegate to find things like Entity.class etc.
- // However, we get errors such as:
+
+ // When using the entity's classloader, we get errors such as:
// NoClassDefFoundError: brooklyn.event.AttributeSensor not found by io.brooklyn.brooklyn-test-osgi-entities
// Building our own aggregating class loader gets around this.
- // But we really should not have to do this! What are the consequences?
+ //
+ // The reason for the error is that the proxy tries to load all classes
+ // referenced from the entity and its interfaces with the single passed loader
+ // while a normal class loading would nest the class loaders (loading interfaces'
+ // references with their own class loaders which in our case are different).
AggregateClassLoader aggregateClassLoader = AggregateClassLoader.newInstanceWithNoLoaders();
- aggregateClassLoader.addFirst(classloader);
--- End diff --
Looking at the code I didn't find a case where spec.getImplementation() != enttiy.getClass() - the entity is always instantiated from getImplementation() if available.
Event if that's the case and the classes (and possibly the classloaders) differ why would we want to use the classloader of a class which didn't instantiate the entity instance?
Looking at this code again I think that it should be improved but in another direction - adding the classloaders of all extended classes and interfaces recursively, not only of the explicitly specified interfaces. WDYT?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15419018
--- Diff: core/src/main/java/brooklyn/util/osgi/Osgis.java ---
@@ -157,138 +160,214 @@ public static Framework newFrameworkStarted(String felixCacheDir, boolean clean,
Map<Object,Object> cfg = MutableMap.copyOf(extraStartupConfig);
if (clean) cfg.put(Constants.FRAMEWORK_STORAGE_CLEAN, "onFirstInit");
if (felixCacheDir!=null) cfg.put(Constants.FRAMEWORK_STORAGE, felixCacheDir);
- cfg.put(Constants.FRAMEWORK_SYSTEMPACKAGES_EXTRA, getBrooklynBootBundles());
FrameworkFactory factory = newFrameworkFactory();
-
+
+ long frameworkInitStart = System.currentTimeMillis();
Framework framework = factory.newFramework(cfg);
try {
framework.init();
- // nothing needs auto-loading, currently (and this needs a new dependency)
- // AutoProcessor.process(configProps, m_fwk.getBundleContext());
+ installBootBundles(framework);
framework.start();
} catch (Exception e) {
// framework bundle start exceptions are not interesting to caller...
throw Exceptions.propagate(e);
}
+ long frameworkInitEnd = System.currentTimeMillis();
+ double frameworkInitSeconds = ((double)frameworkInitEnd - frameworkInitStart) / 1000;
+ LOG.info("OSGi framework started in " + Strings.makeRealString(frameworkInitSeconds, -1, 2, -1) + " seconds.");
+
return framework;
}
- private static String getBrooklynBootBundles() {
+ private static void installBootBundles(Framework framework) {
Enumeration<URL> resources;
try {
- resources = Osgis.class.getClassLoader().getResources("META-INF/MANIFEST.MF");
+ resources = Osgis.class.getClassLoader().getResources(MANIFEST_PATH);
} catch (IOException e) {
throw Exceptions.propagate(e);
}
-
- Collection<String> exportPackages = new ArrayList<String>();
+ BundleContext bundleContext = framework.getBundleContext();
+ Map<String, Bundle> installedBundles = getInstalledBundles(bundleContext);
while(resources.hasMoreElements()) {
URL url = resources.nextElement();
- exportPackages.addAll(getBundleExportedPackages(url));
+ installExtensionBundle(bundleContext, url, installedBundles, getVersionedId(framework));
}
+ }
- Iterator<String> brooklynPackages = Iterators.filter(exportPackages.iterator(), new Predicate<String>() {
- @Override
- public boolean apply(String input) {
- return input.startsWith(BROOKLYN_PACKAGE_PREFIX);
- }
- });
-
- String bootBundles = Joiner.on(",").join(brooklynPackages);
- LOG.debug("Found the following boot OSGi packages: " + bootBundles);
- return bootBundles;
+ private static Map<String, Bundle> getInstalledBundles(BundleContext bundleContext) {
+ Map<String, Bundle> installedBundles = new HashMap<String, Bundle>();
+ Bundle[] bundles = bundleContext.getBundles();
+ for (Bundle b : bundles) {
+ installedBundles.put(getVersionedId(b), b);
+ }
+ return installedBundles;
}
- private static Collection<String> getBundleExportedPackages(URL manifestUrl) {
+ private static void installExtensionBundle(BundleContext bundleContext, URL manifestUrl, Map<String, Bundle> installedBundles, String frameworkVersionedId) {
+ //ignore http://felix.extensions:9/ system entry
+ if("felix.extensions".equals(manifestUrl.getHost())) return;
+
try {
- ManifestHelper helper = ManifestHelper.forManifest(manifestUrl);
- return helper.getExportedPackages();
+ Manifest manifest = readManifest(manifestUrl);
+ if (!isValidBundle(manifest)) return;
+ String versionedId = getVersionedId(manifest);
+ URL bundleUrl = ResourceUtils.getContainerUrl(manifestUrl, MANIFEST_PATH);
+
+ Bundle existingBundle = installedBundles.get(versionedId);
+ if (existingBundle != null) {
+ if (!bundleUrl.equals(existingBundle.getLocation()) &&
+ //the framework bundle is always pre-installed, don't display duplicate info
+ !versionedId.equals(frameworkVersionedId)) {
+ LOG.info("Ignoring duplicate " + versionedId + " from manifest " + manifestUrl + ", already loaded from " + existingBundle.getLocation());
+ }
+ return;
+ }
+
+ byte[] jar = buildExtensionBundle(manifest);
+ LOG.debug("Installing boot bundle " + bundleUrl);
+ //mark the bundle as extension so we can detect it later using the "system:" protocol
+ //(since we cannot access BundleImpl.isExtension)
+ Bundle newBundle = bundleContext.installBundle(EXTENSION_PROTOCOL + ":" + bundleUrl.toString(), new ByteArrayInputStream(jar));
+ installedBundles.put(versionedId, newBundle);
} catch (IOException e) {
LOG.warn("Unable to load manifest from " + manifestUrl + ", ignoring.", e);
} catch (BundleException e) {
LOG.warn("Unable to load manifest from " + manifestUrl + ", ignoring.", e);
}
- return Collections.emptyList();
}
+ private static Manifest readManifest(URL manifestUrl) throws IOException {
+ Manifest manifest;
+ InputStream in = null;
+ try {
+ in = manifestUrl.openStream();
+ manifest = new Manifest(in);
+ } finally {
+ if (in != null) {
+ try {in.close();}
+ catch (Exception e) {};
+ }
+ }
+ return manifest;
+ }
+
+ private static byte[] buildExtensionBundle(Manifest manifest) throws IOException {
+ Attributes atts = manifest.getMainAttributes();
+
+ //the following properties are invalid in extension bundles
+ atts.remove(new Attributes.Name(Constants.IMPORT_PACKAGE));
+ atts.remove(new Attributes.Name(Constants.REQUIRE_BUNDLE));
+ atts.remove(new Attributes.Name(Constants.BUNDLE_NATIVECODE));
+ atts.remove(new Attributes.Name(Constants.DYNAMICIMPORT_PACKAGE));
+ atts.remove(new Attributes.Name(Constants.BUNDLE_ACTIVATOR));
+
+ //mark as extension bundle
+ atts.putValue(Constants.FRAGMENT_HOST, "system.bundle; extension:=framework");
+
+ //create the jar containing the manifest
+ ByteArrayOutputStream jar = new ByteArrayOutputStream();
+ JarOutputStream out = new JarOutputStream(jar, manifest);
+ out.close();
+ return jar.toByteArray();
+ }
+
+ private static boolean isValidBundle(Manifest manifest) {
+ Attributes atts = manifest.getMainAttributes();
+ return atts.containsKey(new Attributes.Name(Constants.BUNDLE_MANIFESTVERSION));
+ }
+
+ private static String getVersionedId(Bundle b) {
+ return b.getSymbolicName() + ":" + b.getVersion();
+ }
+
+ private static String getVersionedId(Manifest manifest) {
+ Attributes atts = manifest.getMainAttributes();
+ return atts.getValue(Constants.BUNDLE_SYMBOLICNAME) + ":" +
+ atts.getValue(Constants.BUNDLE_VERSION);
+ }
/**
* Installs a bundle from the given URL, doing a check if already installed, and
* using the {@link ResourceUtils} loader for this project (brooklyn core)
*/
public static Bundle install(Framework framework, String url) throws BundleException {
- Bundle bundle = framework.getBundleContext().getBundle(url);
- if (bundle != null) {
- return bundle;
+ boolean isLocal = isLocalUrl(url);
+ String localUrl = url;
+ if (!isLocal) {
+ localUrl = cacheFile(url);
}
- // use our URL resolution so we get classpath items
- LOG.debug("Installing bundle into {} from url: {}", framework, url);
- InputStream stream = ResourceUtils.create(Osgis.class).getResourceFromUrl(url);
- return framework.getBundleContext().installBundle(url, stream);
- }
-
- public static class ManifestHelper {
--- End diff --
Is there potential value in keeping this around (and the tests), at least for a bit until we're sure we don't need it? My personal preference is for that, rather than to aggressively delete, because I find myself forgetting where something was done, especially if it's in a deletion, and losing time rewriting. But others feel differently. If you're pretty sure we'll never need this in any case, of course we should kill it now...
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#issuecomment-50191819
Extension bundles look like an elegant solution. I like, though I don't know a whole lot about it.
Agree a slow start is expected with OSGi. Most of the tests run without OSGi for that reason. I don't think this is a problem for normal usage, as java classloading is an expected expensive delay. But if it is, we can resolve then.
Lots of great fixes here. Several comments which should be looked at prior to merging (nothing major however).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15418120
--- Diff: core/src/main/java/brooklyn/entity/proxying/InternalEntityFactory.java ---
@@ -139,15 +138,16 @@ public InternalEntityFactory(ManagementContextInternal managementContext, Entity
}
builder.addAll(spec.getAdditionalInterfaces());
Set<Class<?>> interfaces = builder.build();
-
- // TODO OSGi strangeness! The classloader obtained from the type should be enough.
- // If an OSGi class loader, it should delegate to find things like Entity.class etc.
- // However, we get errors such as:
+
+ // When using the entity's classloader, we get errors such as:
// NoClassDefFoundError: brooklyn.event.AttributeSensor not found by io.brooklyn.brooklyn-test-osgi-entities
// Building our own aggregating class loader gets around this.
- // But we really should not have to do this! What are the consequences?
+ //
+ // The reason for the error is that the proxy tries to load all classes
+ // referenced from the entity and its interfaces with the single passed loader
+ // while a normal class loading would nest the class loaders (loading interfaces'
+ // references with their own class loaders which in our case are different).
AggregateClassLoader aggregateClassLoader = AggregateClassLoader.newInstanceWithNoLoaders();
- aggregateClassLoader.addFirst(classloader);
--- End diff --
could it ever be the case that there is a classloader for `spec.getImplementation()` which is different to `entity.getClass()` ? i don't think so but not sure. unless you're sure, it might be worth a check `if (spec.getImplementation()!=null && !spec.getImplementation().equals(entity.getClass()) { /* log warn and add the original spec.getImplementation().getClassLoader() */ }.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15458170
--- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
@@ -120,6 +131,11 @@ public void registerBundle(String bundleUrl) {
} catch (Exception e) {
Exceptions.propagateIfFatal(e);
bundleProblems.put(bundleUrlOrNameVersionString, e);
+
+ Throwable cause = e.getCause();
+ if (cause != null && cause.getMessage().contains("Unresolved constraint in bundle")) {
+ log.warn("Unresolved constraint.", e);
--- End diff --
Agree, better not have the stacktrace in the console, but still I would print the nested exception's message:
log.warn("Unresolved constraint resolving OSGi bundle "+bundleUrlOrNameVersionString+" to load "+type+": "+cause);
Not terribly proud with this code, but couldn't find a better approach to bring out this important cause of ClassNotFoundException.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15458566
--- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
@@ -108,11 +108,22 @@ public void registerBundle(String bundleUrl) {
if (bundleNameVersion==null) {
bundleNameVersion = bundleUrlOrNameVersionString;
}
-
+
Maybe<Bundle> bundle = Osgis.getBundle(framework, bundleNameVersion);
if (bundle.isPresent()) {
- @SuppressWarnings("unchecked")
- Class<T> clazz = (Class<T>) bundle.get().loadClass(type);
+ Bundle b = bundle.get();
+ Class<T> clazz;
+ //Extension bundles don't support loadClass.
+ //Instead load from the app classpath.
+ if (Osgis.isExtensionBundle(b)) {
+ @SuppressWarnings("unchecked")
+ Class<T> c = (Class<T>)Class.forName(type);
--- End diff --
Extension bundles are almost identical to the previous approach we used. Their goal is to direct Felix to use the app's classloader. The only difference is that now the container knows the name:version of the code that is on the classpath and will deny requests to install external bundles with matching name:version.
Felix still doesn't know for the existence of our bundle-A, we just tell it that the Export-Package list in the extension bundle should be looked for in the app's classpath. We don't ever install bundle-A in Felix. There is a hardcoded check that throws on an attempt to bundle.loadClass from extension bundle.
With that in mind, yes the code deliberately ignores extension bundles and goes directly for the app's classloader.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#issuecomment-51149454
explanation and using classloading hierarchy makes sense - merging (will fix merge conflict)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by ahgittin <gi...@git.apache.org>.
Github user ahgittin commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/90#discussion_r15418319
--- Diff: core/src/main/java/brooklyn/management/ha/OsgiManager.java ---
@@ -120,6 +131,11 @@ public void registerBundle(String bundleUrl) {
} catch (Exception e) {
Exceptions.propagateIfFatal(e);
bundleProblems.put(bundleUrlOrNameVersionString, e);
+
+ Throwable cause = e.getCause();
+ if (cause != null && cause.getMessage().contains("Unresolved constraint in bundle")) {
+ log.warn("Unresolved constraint.", e);
--- End diff --
stack trace probably not helpful, so I would say
log.warn("Unresolved constraint resolving OSGi bundle "+bundleUrlOrNameVersionString+" to load "+type+": "+e);
if (log.isDebugEnabled()) log.debug("Trace for OSGi resolution failure", e);
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: OSGi class loading fixes
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/90
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---