Improve loading of ReactiveStreamsFactory and ReactiveStreamsEngine to be in line with the solution in other specs

eclipse / microprofile-reactive-streams-operators

Microprofile project

Apache License 2.0

79 stars 33 forks source link

Improve loading of ReactiveStreamsFactory and ReactiveStreamsEngine to be in line with the solution in other specs #103

Closed OndroMih closed 5 years ago

OndroMih commented 5 years ago

Currently, ReactiveStreamsFactory is loaded using service loader in a private factory() method. Other specs do something similar to provide an implementation (Config in ConfigProviderResolver, REST client, etc.)

Other specs that use this pattern also do the following, which the current solution in ReactiveStreamsFactory.factory() does not:

load the implementation using the thread contect classloader instead of the default classloader
provide means to set an implementation directly and bypass service loader, this is to support environments where service loader isn't supported, mainly OSGi

An example of a solution, which meets the above 2 points, is the ConfigProviderResolver.instance() in the Config spec.

OndroMih commented 5 years ago

@cescoffier @jroper, it should be enough to copy&paste from Config spec. I'd like to do it if I find time. @Emily-Jiang, maybe you could do it too, if you can find time for it sooner than me?

cescoffier commented 5 years ago

I don't mind using the approach from the other spec. @jroper WDYT?

cescoffier commented 5 years ago

BTW, the issue also affects ReactiveStreamsEngine.

cescoffier commented 5 years ago

I've updated the title to reflect the 2 cases.

hutchig commented 5 years ago

I have no strong opinion on this, it does make sense that the different specs have the same approach where possible.

If you ever need to get round this problem, you might find it interesting that in WebSphere OpenLiberty (which is all OSGi) we currently make bnd files include:

https://github.com/OpenLiberty/open-liberty/blob/integration/dev/cnf/resources/bnd/app-resources.bnd after setting app-resources to the service-load file and then https://github.com/OpenLiberty/open-liberty/blob/integration/dev/com.ibm.ws.classloading/src/com/ibm/wsspi/classloading/ResourceProvider.java will make the resources service loadable via the Java service loading mechanism. not saying that is an alternative solution - just for your curiosity :-).

Would setting the implmentation affect the implementation Injected as well as that service loaded?

cescoffier commented 5 years ago

@hutchig There are many ways to deal with SPI in OSGi environment. I tend to recommend extender patterns (at the module layer) or whiteboard patterns (at the service layer). For this issue, we should apply what the other specs have done. Allowing to pass a specific instance can also be useful in environments with limited SPI feature or reflection facilities (such as GraalVM/SubstrateVM - even if SPI support has been added recently).

cescoffier commented 5 years ago

@OndrejM - https://github.com/eclipse/microprofile-reactive-streams/pull/104.

hutchig commented 5 years ago

@cescoffier Clement, I bow to your OSGi fu...and +1 on #104. (for my own reference/learning: https://www.osgi.org/wp-content/uploads/whiteboard1.pdf https://osgi.org/specification/osgi.cmpn/7.0.0/service.loader.html)

cescoffier commented 5 years ago

@hutchig I love the ASCII art in the bnd file! The approach you use Declarative Services so it exposes the SPI implementation as a service. It's actually pretty nice as it does not require using Bundle.getClassLoader() which has security implications.

In the PR, it just allows "any" bundle importing the package to set the instance. So, it can be used in combination with the Declarative Service approach.

cescoffier commented 5 years ago

PR merged in master.