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

Seeking various RxJavaPlugins via java.util.ServiceLoader #2643

Closed
wants to merge 2 commits into from

Conversation

jtulach
Copy link

@jtulach jtulach commented Feb 10, 2015

The standard JDK way for registering plugins is via java.util.ServiceLoader. I can see RxJava invented few home-made solutions to such registration, but all of them require some configuration (set a property or invoke some bootstrap code). I'd like to extend the set of possible registration styles to ServiceLoader as well: all that is needed with ServiceLoader is to add a JAR with standard META-INF/services/pgk.iterface.name file on classpath which I consider the least intrusive way to register a plugin.

Btw. my notes on service loader & related stuff: http://wiki.apidesign.org/wiki/ServiceLoader

@benjchristensen
Copy link
Member

I'm open to considering additional approaches, but I have concerns with this dynamic class path approach and reasons why this was never pursued. In large apps with transitive dependencies it is a mess to have behavior change just because a new jar ends up in a classpath. For example, any application using RxJava could be affected just because a new 3rd party library is pulled in. A 3rd party library can still manually register a plugin today, but it requires code making conscious effort, and being invoked before anything else using RxJava (which is generally harder to do as a 3rd party library). With classpath scanning it is far easier for something to make its way in, and initialize before anything else, because RxJava would just pick it up when it is first touched from the classpath even if normally the app was operating without plugins.

cc @mattrjacobs @abersnaze @akarnokd What are your thoughts on this?

@jtulach What is the expected behavior of your proposal if someone has already registered a plugin? If multiple providers end up in the classpath? What about the implication of plugins easily getting initialized by an app now without awareness?

@akarnokd
Copy link
Member

I agree with @benjchristensen: this is less controllable.

@jtulach
Copy link
Author

jtulach commented Feb 11, 2015

What is the expected behavior of your proposal if someone has already registered a plugin?

Up to you. Hearing your concerns the registration via property or call to register method should probably take precedence.

If multiple providers end up in the classpath?

The current patch uses the first one returned by ServiceLoader, which in the currently implementation is the first one in order of JARs specified by a classpath.

any application using RxJava could be affected just because a new 3rd party library is pulled in.

I usually value API changes based on usecases and try to optimize for the most common one. Here is the usecase I consider most common:

  • Somebody creates a class that implements RxJavaObservableExecutionHook. Why? Probably to register it as a provider.
  • Somebody includes JAR with such class in own application. Why? Probably to use it as a provider.

Of course there are other usecases: For example somebody wants to use the implementation of RxJavaObservableExecutionHook only on Mondays or Tuesdays. But I consider such usecase much less likely than the "most common" one.

I believe the ServiceLoader, possibly with combination of ServiceProvider annotation from http://bits.netbeans.org/nexus/content/groups/netbeans/org/netbeans/api/org-openide-util-lookup/RELEASE80/, is best suited to satisfy the "most common" usecase. Just implement and annotate your class (annotation generates META-INF/services/.... file):

@ServiceProvider(service=RxJavaObservableExecutionHook.class)
public final class MyExecutionHook implements RxJavaObservableExecutionHook {
 ...
}

and package your implementation into own JAR file. Once some application includes your JAR file, the hook gets activated. Really slick way to satisfy the "most common" usecase. Plus please note that other, more flexible - e.g. just on Tuesdays - registrations are still possible and can take precedence, if used.

Re. "transitive dependencies" - in environments I am aware of most, the JAR with registered implementation would be "a leaf" JAR - there is no reason for some library except the final application project to have dependency on it. I have not "accidentally" enabled an implementation yet - but should such situation happen excluding a particular dependency is always a solution.

@jtulach
Copy link
Author

jtulach commented Feb 16, 2015

In light of web applications and containers mentioned in #2573 the request to support ServiceLoader configuration seems to make even more sense than I originally thought. Seeking for META-INF/services/smthng is a natural way to configure isolated classloader spaces without executing any client code.

To give a bit more context why I am in need of ServiceLoader-like registration: I am trying to bring RxJava into DukeScript. E.g. write a Java application once and then execute it on any device (desktop, iOS, Android) and even a (plugin-less) browser - see my demo at http://xelfi.cz/RxJava/0.1/. Desktop, iOS, Android seem to handle standard RxJava scheduler implementation OK, but the browser version (executed via Bck2Brwsr VM) does not handle threads - e.g. needs to reconfigure the schedulers.

Preferably the reconfiguration could be handled without any code (otherwise we don't really have single code base for all devices). By including special JAR with META-INF/services registrations on Bck2Brwsr VM classpath and with help of this pull request I can achieve that.

@benjchristensen
Copy link
Member

@mattrjacobs @daschl I'd appreciate your input on this.

@benjchristensen
Copy link
Member

@jtulach My current hangup on this is that your assertion of "most common" is based on your perception of common and seems to dismiss the "most common" case of transitive dependencies in large organizations where multiple service provider jars could easily end up in a classpath. In 2 years of usage this is the first time this has become a point of discussion, so it is not a significant need. Thus I'm going to move slowly and many things are higher priority.

@mattrjacobs
Copy link
Contributor

I agree with the initial comments. In many projects I've worked on, the classpath is fluid enough that implicit plugin resolution is fairly dangerous, and I would much prefer an explicit resolution strategy (i.e. invoke a Java method to register a plugin)

@daschl
Copy link
Contributor

daschl commented Mar 5, 2015

Actually I'm in line with @benjchristensen and @mattrjacobs on this one. I'm not sure this enhances the flexibility of the system and introduces lots of holes that are hard to uncover until you fall in one.

Let me propose something else: why don't you just write your own little helper class that grabs it from the ServiceLoader and applies the plugins manually?

@abersnaze
Copy link
Contributor

I'm in favor of the ServiceLoader approach. Sometimes it isn't possible to set hooks early enough. I've run into this here at Netflix where I had an execution hook that was working suddenly stop. It was because a new version base framework that we use started using Rx before our code got a chance to run.

I had to switch to setting the property rxjava.plugin.RxJavaObservableExecutionHook.implementation to the class name of my hook.

I see the ServiceLoader method as a more formal way of declaring a provider than the convention of putting a class name in a system property. It also has the added benefit that if multiple service providers are found they can be detected and an error can be thrown (or composed to into one service). Right now if the property is set multiple times one will just silently be dropped without any warning.

@jtulach
Copy link
Author

jtulach commented Mar 6, 2015

What @abersnaze describes is a classical problem with singletons (RxJava hooks are de-facto singletons). If you mix initialization code with application code you may easily access the singletons before they are initialized. Many people observed that and concluded that singletons are bad. However it is not the whole truth as I analysed in an essay few years ago. If you turn your singletons into injectable singletons - e.g. make sure they are inherently initialized once accessed, the system becomes more stable.

All you need is a declarative (e.g. non-procedural) way of registering singletons/services. _ServiceLoader_ is one of the options and benefits from the fact that once an application is started its classpath is usually quite immutable - e.g. the configuration is defined and set at start time - sooner before any application code is executed.

@akarnokd
Copy link
Member

This is quite old. What's the verdict on this PR?

@ghost
Copy link

ghost commented Jun 19, 2015

I agree with @jtulach. His example of injectable singletons follows a bit the initialization-on-demand holder idiom, the implementation class is initialized only at the very first time you try to get an instance of it, it's done lazily.

@stealthcode
Copy link

@abersnaze and I did an short experiment to test out the options here. Firstly, the ServiceLoader.load() method returns an iterator of implementations. This iterator would match any configuration in the order determined by the classpath. This is usually not manually crafted or controlled so you may not have deterministic ordering in the iterator.

Also there is a ClassNotFoundException issue when using the system properties (in a corner case pathological usage). This issue occurs when a jar that contains the class that implements the hooks is defined in a jar that is loaded in a child class loader off the system class loader. If a class defined in the system classloader were the first one to load and use RxJava then the magic system property would be loaded by a class that cannot resolve any classes loaded in the child class loader. It is not entirely clear to me if the service loader approach would also suffer from this issue (i.e. would the service loader have all implementations defined for any class loader context).

That said we can see non-deterministic behavior problems with both approaches.

@jtulach
Copy link
Author

jtulach commented Jun 23, 2015

Re. "This is quite old" - yes, it is. I got a feeling the discussion diverged into wrong direction and I stopped pushing the change forward.

Re. "the ServiceLoader.load() method returns an iterator of implementations" - yes, that is true. The current patch would just silently ignore subsequent entries. You are right, it could be made more robust. Here is the most simple take on that: bf98531 - it will shield users from any problems related to classpath elements ordering issues by providing an early warning.

@benjchristensen
Copy link
Member

Anyone other than those already involved in this thread have opinions or information that can help sway the discussion in a direction? It seems there are not very good options for this.

@akarnokd
Copy link
Member

I'm not convinced of this PR and since no one else has commented in the last month, I'm closing this PR.

@akarnokd akarnokd closed this Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants