Osgi environments (like GlassFish) have trouble with BuildServicesResolver

[arjantijms]

jakarta.enterprise.inject.build.compatible.spi.BuildServicesResolver contains the following code to load an implementation of BuildServices.class

private static void discoverFactories() {
        Set<BuildServices> factories = new TreeSet<>(
                Comparator.comparingInt(BuildServices::getPriority).reversed());

        ServiceLoader<BuildServices> loader = SecurityActions.loadService(
                BuildServices.class, BuildServicesResolver.class.getClassLoader());

The problem here is that it loads this using the class loader from BuildServicesResolver, which in an OSGi environment is the bundle corresponding to the CDI API jar, e.g. the bundle identified by "jakarta.enterprise.cdi-api [131]" in GlassFish.

The bundle itself is not able (allowed) to load META-INF/services/jakarta.enterprise.inject.build.compatible.spi.BuildServices from other bundles or from anywhere on the class path, as it would only look into its own bundle (where it's obviously not found).

I think the best way to go about this is trying the context class loader as well. The environment can then set an appropriate class loader that is both OSGi aware and aware of the class path, which is able to deal with this. Alternatively, it might work if the CDI API jar defines the need for a server loader service in its manifest:

Require-Capability: osgi.extender;
      filter:="(osgi.extender=osgi.serviceloader.processor)",
    osgi.serviceloader;
      filter:="(osgi.serviceloader=jakarta.enterprise.inject.build.compatible.spi.BuildServices)";
      cardinality:=multiple

(I'd have to test whether this indeed works in GlassFish)

[starksm64]

We are only using the the API artifact class loader for the CDIProvider as well, and I see no usage of the thread context class loader in the API classes. How has this not come up before given several OSGi based server use these jars? Are they replacing the loading code? I would agree that normally one would use the TCCL first and only use the API jar ClassLoader if it was null.

…

On May 21, 2022 at 9:49:22 AM, Arjan Tijms ***@***.***> wrote: jakarta.enterprise.inject.build.compatible.spi.BuildServicesResolver contains the following code to load an implementation of BuildServices.class private static void discoverFactories() { Set<BuildServices> factories = new TreeSet<>( Comparator.comparingInt(BuildServices::getPriority).reversed()); ServiceLoader<BuildServices> loader = SecurityActions.loadService( BuildServices.class, BuildServicesResolver.class.getClassLoader()); The problem here is that it loads this using the class loader from BuildServicesResolver, which in an OSGi environment is the bundle corresponding to the CDI API jar, e.g. the bundle identified by "jakarta.enterprise.cdi-api [131]" in GlassFish. The bundle itself is not able (allowed) to load META-INF/services/jakarta.enterprise.inject.build.compatible.spi.BuildServices from other bundles or from anywhere on the class path, as it would only look into its own bundle (where it's obviously not found). I think the best way to go about this is trying the context class loader as well. The environment can then set an appropriate class loader that is both OSGi aware and aware of the class path, which is able to deal with this. Alternatively, it might work if the CDI API jar defines the need for a server loader service in its manifest: Require-Capability: osgi.extender; filter:="(osgi.extender=osgi.serviceloader.processor)", osgi.serviceloader; filter:="(osgi.serviceloader=jakarta.enterprise.inject.build.compatible.spi.BuildServices)"; cardinality:=multiple (I'd have to test whether this indeed works in GlassFish) — Reply to this email directly, view it on GitHub <#615>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACRDMW45MV7LVF36S7AOLTVLDZXFANCNFSM5WR5XHCA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[arjantijms]

How has this not come up before given several OSGi based server
use these jars? Are they replacing the loading code?

Not sure, I would have to look into the exact case and probably need to do some debugging to see why it does work there.

I would agree that normally one would use the TCCL first and only use the
API jar ClassLoader if it was null.

Maybe best if I did a PR with the changes for that? If CDI could then at least stage a 4.0.1 or 4.0sp1 jar that would be super. I think there were some other small things pending for a 4.0sp1 release of the API jar as well.

[Ladicek]

I wrote that code by copying the CDI class (which loads CDIProvider) and then simplifying a little. I did notice the code in CDI doesn't use TCCL and thought it's a bit weird, but I said to myself -- this must already work everywhere. So I didn't attempt to load from TCCL, even if that would be the first thing I'd normally do.

[manovotn]

We are only using the the API artifact class loader for the CDIProvider as
well, and I see no usage of the thread context class loader in the API
classes. How has this not come up before given several OSGi based server
use these jars?

I had the exact same thought when I read the issue.
If we could learn why it works in some case and not in another, that would be nice.

Also, OpenLiberty uses OSGi and they didn't come across this issue and said they have a fully working server now. Maybe @Azquelt knows more?

[arjantijms]

Also, OpenLiberty uses OSGi and they didn't come across this issue and said they have a fully working server now. Maybe @Azquelt knows more?

I might just have found it in GlassFish:

<name>cdi-api Fragment</name>
<description>This fragment bundle extends the cdi-api bundle to import additional packages into the cdi-api bundle.</description>

<manifestEntries>
    <Bundle-SymbolicName>${project.groupId}.${project.artifactId}</Bundle-SymbolicName>
    <Bundle-ManifestVersion>2</Bundle-ManifestVersion>
    <Fragment-Host>jakarta.enterprise.cdi-api</Fragment-Host>
    <Import-Package>org.glassfish.weld</Import-Package>
</manifestEntries>

unzip -l cdi-api-fragment.jar 
Archive:  cdi-api-fragment.jar
  Length      Date    Time    Name
---------  ---------- -----   ----
        0  05-23-2022 00:37   META-INF/
      376  05-23-2022 00:37   META-INF/MANIFEST.MF
        0  05-23-2022 00:37   META-INF/services/
        0  05-23-2022 00:37   META-INF/maven/
        0  05-23-2022 00:37   META-INF/maven/org.glassfish.main.web/
        0  05-23-2022 00:37   META-INF/maven/org.glassfish.main.web/cdi-api-fragment/
    35084  05-23-2022 00:37   META-INF/LICENSE.md
     4734  05-23-2022 00:37   META-INF/NOTICE.md
       40  05-23-2022 00:37   META-INF/services/jakarta.enterprise.inject.spi.CDIProvider
     2563  05-04-2022 21:04   META-INF/maven/org.glassfish.main.web/cdi-api-fragment/pom.xml
       82  05-23-2022 00:37   META-INF/maven/org.glassfish.main.web/cdi-api-fragment/pom.properties

So seemingly it "hacks" into the CDI bundle to add the service file to it, and also adds the org.glassfish.weld import to it.

Hmmm...

[Azquelt]

OpenLiberty uses the same approach with a fragment that @arjantijms described for GlassFish.

It does seem like a bit of a hack, but at least it doesn't require polluting the API bundle itself. I'm not sure there's really a nice way of making ServiceLoader work with OSGi.

[arjantijms]

@Azquelt I'll use the same "hack" then for this issue here. What do you think, is it still a good idea to merge this PR (and update CDI as well)?

I'm not sure there's really a nice way of making ServiceLoader work with OSGi.

Did you ever tries something like this for the consumer?

Require-Capability: osgi.extender;
      filter:="(osgi.extender=osgi.serviceloader.processor)",
    osgi.serviceloader;
      filter:="(osgi.serviceloader=jakarta.enterprise.inject.build.compatible.spi.BuildServices)";
      cardinality:=multiple

(and then a similar entry for the producer/exporter of the service file)

[manovotn]

Resolved by #617