Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Service loader updates #65

Merged
merged 14 commits into from
Mar 30, 2016
Merged

Conversation

geomacy
Copy link
Contributor

@geomacy geomacy commented Mar 15, 2016

This PR contains a number of changes that are a contribution to getting Brooklyn running on Karaf.

The Brooklyn code uses a ServiceLoader in a small number of places to look up a variety of classes. In OSGI this doesn't work as not everything is in the same classloader. These changes move the lookups into a utility that will first check OSGI service registry for the lookup, and, if the OSGI environment is unavailable, will fall back to the previous behaviour of using the ServiceLoader, for backward compatibility.

@CMoH
Copy link
Contributor

CMoH commented Mar 16, 2016

I haven't tested it, but it looks like it would work.

One comment though: when calling BundleContext.getService() the reference count for the service in question is increased, and without any calls to BundleContext.ungetService() the only moment it will drop to zero is when the client bundle (which is brooklyn-api here) is unloaded [1]. I don't think it will be a problem for now, but we may hit some problems in the future.

One thought is to cache the services obtained by BundleContext.getService() inside FrameworkLookup so that the count does not up for every lookup. Or maybe these lookups are rare enough for this not to matter?

I haven't tried this yet, but I'm looking at using Aries proxies [2] for the same reason in another project. However, I'm leaning towards injecting needed services in a registry of proxies that avoids the need to call ungetService, and leaving clients in blocked calls if the provider service gets unregistered while in use (i.e in the grace period). In my case, however, all services expose the same interface, which makes the code easy. But I'm still throwing the idea on the table - maybe it spawns ideas in others.

[1] https://osgi.org/javadoc/r4v43/core/org/osgi/framework/BundleContext.html#getService%28org.osgi.framework.ServiceReference%29
[2] https://github.com/apache/aries/blob/trunk/proxy/proxy-api/src/main/java/org/apache/aries/proxy/ProxyManager.java

@geomacy
Copy link
Contributor Author

geomacy commented Mar 16, 2016

Thanks for this good point @CMoH. I think in this case, where we are using thegetService as an alternative to the ServiceLoader.load(), that the fact that the reference count will only increase is not likely to cause a problem, i.e. given how we are using it. It would certainly mean that the FrameworkLookup class wouldn't be suitable for use in a situation where you want to take account of services coming and going, and explicitly avoid using the service when its reference count has gone to zero. I think this is worth adding as a note in the Javadoc for this class, which I will do right now.

@CMoH
Copy link
Contributor

CMoH commented Mar 16, 2016

Forgot to mention the use of ServiceTracker - which is the preferred alternative to using ServiceReferences, but someone reminded me. Just adding here for reference.

@geomacy
Copy link
Contributor Author

geomacy commented Mar 16, 2016

My thought was that, while ServiceTracker in general may be preferred to ServiceReference, it is actually not best suited in this use case, where in fact I don't want to track the service (just as one wouldn't track the result of a ServiceLoader.load()). If we were to use ServiceTracker then we would have to take care of the open/close of the tracker, which would mean maintaining that state, which would mean not using these static methods but perhaps making the FrameworkLookup be a service in its own right, which would have to be obtained from OSGI (and maybe tracked itself!). The client code in this use case isn't expecting this sort of behaviour. What do you think?

@CMoH
Copy link
Contributor

CMoH commented Mar 16, 2016

Well, my comment on ServiceTracker was about completing the list of alternatives, not about the current solution - which I think works just fine for the intended purpose.

* @param <T> The type for the class.
* @return A maybe of the instance found in the framework.
*/
public static <T> Maybe<T> lookup (Class<T> clazz, ClassLoader loader) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal preference, could cut this file in half if supporting only the lookupAll case with users getting the first entry where needed.

@neykov
Copy link
Member

neykov commented Mar 17, 2016

Minor comments only, good to merge.

If we have chosen to use factories for the services instead of exporting them directly, then the reference counting issue wouldn't be a problem (they would be freed immediately). Could be changed in future when need arises.

Necessary to avoid NPE at line 135 and 160 if the class being looked up is in a bundle that hasn't started yet.
We don't really need to get the bundle context from that class, any suitable one will do, and we know that
the bundle we live in has started!
@neykov
Copy link
Member

neykov commented Mar 18, 2016

Just came by how jclouds approaches the same problem. Here it is just FYI.
In an activator they subscribe to bundle add/remove events. When a bundle is added they scan it for /META-INF/services/org.jclouds.providers.ProviderMetadata, instantiating the classes inside using the bundle class loader. Discovered instances are recorded and fetched when needed.
See MetadataBundleListener and ProviderRegistry for further details.

@neykov neykov mentioned this pull request Mar 28, 2016
ahgittin added a commit to ahgittin/brooklyn-server that referenced this pull request Mar 28, 2016
@asfgit asfgit merged commit b2df8dd into apache:master Mar 30, 2016
@geomacy geomacy deleted the service-loader-updates branch April 4, 2016 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants