Fix loading BuildServices for OSGi

[arjantijms]

Fixes #615

Signed-off-by: Arjan Tijms [email protected]

[Ladicek]

Looks good to me, but I'd sleep better if I knew why the same change isn't necessary to load CDIProvider in the CDI class :-)

[Azquelt]

I'm a little unsure on this. Generally the TCCL is used to access application classes and we don't want the user providing their own BuildServices implementation, though I can also see that it's necessary if you wanted to use OSGi Service Loader Mediation.

@Ladicek CDI has a setCDIProvider() method. I know in OpenLiberty we call that when our implementation bundle activates and the method specifies that you can only set the provider once, so no-one can change it later. A similar approach could be used for BuildServicesResolver, though I know microprofile have had arguments over whether these sorts of methods are a good idea or not.

[Azquelt]

Second thought, it might at least be worth checking our own classloader before the TCCL to avoid the risk of implementations which aren't using OSGi service loader mediation finding an implementation provided by the application.

[Emily-Jiang]

I feel this is a guessing exercise regarding which classloader to use. Does it make sense to pass in the classloader instead? In MP Config, we have this approach, which allows the implementors to pass in the classloader they want to use.

[OndroMih]

Hi @Emily-Jiang , how can that approach be used if the method is private? Didn't you rather mean using the setInstance method, which can be called before any call to instance() to set a specific instance rather than loading it via the default service loader mechanism?

This is the approach in MicroProfile Config in short:

• By default, an implementation is found using service loader, it will use the classloader of the class. This happens by default if ConfigProvider.getConfig() is called
• If a runtime wants to pass a specific implementation, it will call ConfigProviderResolver.setInstance(impl) with that implementation when it boots. This works only if this is called before any call to ConfigProvider.getConfig().

MP Config doesn't rely on the thread-context classloader because the same effect can be achieved by the call to setInstance method and it's enough to do it once in the beginning. With a thread context classloader solution, thread context classloader would have to be set each time an application can potentially trigger initialization of the SPI, which is an overhead for some implementations that don't normally use thread-context classloader.

[Emily-Jiang]

Hi @Emily-Jiang , how can that approach be used if the method is private? Didn't you rather mean using the setInstance method, which can be called before any call to instance() to set a specific instance rather than loading it via the default service loader mechanism?

This is the approach in MicroProfile Config in short:

* By default, an implementation is found using service loader, it will use the classloader of the class. This happens by default if ConfigProvider.getConfig() is called

* If a runtime wants to pass a specific implementation, it will call ConfigProviderResolver.setInstance(impl) with that implementation when it boots. This works only if this is called before any call to ConfigProvider.getConfig().

MP Config doesn't rely on the thread-context classloader because the same effect can be achieved by the call to setInstance method and it's enough to do it once in the beginning. With a thread context classloader solution, thread context classloader would have to be set each time an application can potentially trigger initialization of the SPI, which is an overhead for some implementations that don't normally use thread-context classloader.

@OndroMih you were right. Open Liberty uses setInstance() and IIRC Payara also uses that. It might be easier to have the similar approach introduced for BuildServicesResolve. All I suggest is not to keep on guessing which classloader to use for OSGi environment espeically.

[Ladicek]

I agree BuildServicesResolver should have a public static void setBuildServices(BuildServices instance) method (and the class should be public). That is probably the best solution for all kinds of integrators, while service loader lookup clearly only works in very simple classloading architectures.

[arjantijms]

That is probably the best solution for all kinds of integrators

Might be indeed. Shall we close this PR then? Or does anyone think an attempt to use the context class loader still has merit?

[OndroMih]

I personally think that a thread-context classloader still has some value, it allows to initialize the BuildServices instance only if the application needs it. So I'd recommend supporting it too, because it's very simple code and some implementations might find it more convenient than using the setter.

But I would also add a setter so that the following alogorithm is used:

• if instance set by a setter, use the instance
• if thread context classloader set, use service loader using that classloader
• use service loader using the current classloader

Such a solution is still very simple and provides enough flexibility for implementors to hook their own provider into the API.

[Ladicek]

I'd rather keep CDI and BuildServicesResolver behavior identical. If we want to use TCCL for service loader lookup, in my opinion, the CDI class should do that too.

[manovotn]

The underlying issue was instead resolved by #617