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

Jackson bundles are missing OSGi's osgi.serviceloader metadata #768

Closed
chrisr3 opened this issue Jun 16, 2022 · 22 comments
Closed

Jackson bundles are missing OSGi's osgi.serviceloader metadata #768

chrisr3 opened this issue Jun 16, 2022 · 22 comments
Milestone

Comments

@chrisr3
Copy link
Contributor

chrisr3 commented Jun 16, 2022

Bundles need to include extra metadata to support java.util.ServiceLoader inside an OSGi framework. Jackson's bundles are all missing this metadata.

In theory, it should just be a matter of applying Bnd's @ServiceConsumer and @ServiceProvider annotations in the correct places as described here, where these annotations can be provided like this:

diff --git a/base/pom.xml b/base/pom.xml
index 6235a67..4972957 100644
--- a/base/pom.xml
+++ b/base/pom.xml
@@ -30,9 +30,17 @@ of Jackson: application code should only rely on `jackson-bom`
          parent pom, and then also allow override as need be
       -->
     <jackson-bom.version>${project.parent.version}</jackson-bom.version>
+
+    <version.bnd.annotation>6.3.1</version.bnd.annotation>
   </properties>
 
   <dependencies>
+    <dependency>
+      <groupId>biz.aQute.bnd</groupId>
+      <artifactId>biz.aQute.bnd.annotation</artifactId>
+      <version>${version.bnd.annotation}</version>
+      <scope>provided</scope>
+    </dependency>
     <dependency> <!-- all components use junit for testing -->
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>

I am struggling to wrap my head around your myriad of separate repositories, and so you may have a better way of providing this dependency.

@cowtowncoder
Copy link
Member

Being bit of OSGi newbie, quick question: is this really needed for jackson-core which does not provide Service of any type -- meaning there is no life-cycle or such. It is simply a library with visibility as defined by exports (and theoretically imports but in practice having few).

So what would be the meaning of ServiceLoader for this particular component? (or for others, jackson-databind)
It sounds like there is a problem to solve, I just want to understand what that is.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Jun 21, 2022

So what would be the meaning of ServiceLoader for this particular component? (or for others, jackson-databind) It sounds like there is a problem to solve, I just want to understand what that is.

jackson-core contains META-INF/services/com.fasterxml.jackson.core.JsonFactory, which means that it "provides" the classes inside this file as ServiceLoader "services". As a rule, the classes listed inside META-INF/services/* files need to be annotated as @ServiceProvider, and classes which directly invoke ServiceLoader.load(...) need to be annotated as @ServiceConsumer. These annotations instruct the Maven plugin to generate extra OSGi metadata inside the bundles, which can be used by OSGi's Service Loader Mediator

The story becomes more complicated with jackson-databind because e.g. jackson-databind-xml uses XMLInputFactory.newFactory() etc which use ServiceLoader.load(...) indirectly. The Apache Aries SPI-Fly bundle supports extra OSGi metadata for cases like these. I know there is an issue with at least jackson-databind-xml and woodstox-core because I have tripped over it.

@cowtowncoder
Copy link
Member

Thank you @chrisr3! Interesting that this hadn't been reported before but I guess better late than never :)

@chrisr3
Copy link
Contributor Author

chrisr3 commented Jun 21, 2022

Thank you @chrisr3! Interesting that this hadn't been reported before but I guess better late than never :)

No worries. The maven-bundle-plugin will do the "heavy lifting" for you; all you need to do is apply the @ServiceProvider and @ServiceConsumer annotations. The @ServiceProvider ones are easier because the classes are by definition listed inside each bundle's META-INF/services/* files. However, as I understand SPI, I don't see how these woodstox ones can be correct 😕:

org.codehaus.stax2.validation.XMLValidationSchemaFactory.dtd
org.codehaus.stax2.validation.XMLValidationSchemaFactory.relaxng
org.codehaus.stax2.validation.XMLValidationSchemaFactory.w3c

I'll also mention that I haven't found any direct invocations of ServiceLoader.load(...) anywhere in Jackson yet, which would mean there are no suitable candidates for @ServiceConsumer. But even just adding appropriate @ServiceProvider annotations would help.

@cowtowncoder
Copy link
Member

Yeah I wouldn't count on Woodstox having it correct. Would obv. love help there too if someone could point out better ways.

As to Jackson, there is no ServiceLoader usage (or even JDK SPI outside of OSGi): all OSGi metadata is meant for "external consumption" (by something else using Jackson).
I guess there can't be any external usage either since metadata is missing tho. :)

@chrisr3
Copy link
Contributor Author

chrisr3 commented Jun 22, 2022

I guess there can't be any external usage either since metadata is missing tho. :)

There are "ways" around bundles having broken metadata. However, it's much easier if the bundles are fixed at source 😉.

@cowtowncoder
Copy link
Member

note to self: Woodstox PR FasterXML/woodstox#151 should work as template here.
Needs some care wrt merging from 2.x to 3.0, but otherwise fine.
Going forward I assume jackson-databind would benefit from the same process, after which dataformat modules (jackson-dataformats-binary, jackson-dataformats-text, jackson-dataformat-xml) likewise.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Jun 28, 2022

note to self: ..., after which dataformat modules (jackson-dataformats-binary, jackson-dataformats-text, jackson-dataformat-xml) likewise.

jackson-dataformat-xml will also require upgrading to use woodstox-core v6.3.0, of course 😉.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jun 28, 2022

Yes. I'll go ahead and do that now, then:

FasterXML/jackson-dataformat-xml#536

EDIT: Done.

@rakeshk15
Copy link

This change will require the Aries SPIFly bundle and the jackson-core bundle will not resolve wherever Aries SPIFly bundle is missing.

Not everyone has the Aries SPIFly available at runtime, so, this is a problematic upgrade from v2.13.x to v2.14.0

As v2.14.0 is a minor update and should be backward compatible but it is not as per current requirements of Aries SPIFly bundle.

Also, people hardly use ServiceLoader mechanism in OSGi because of class loader issues.

@chrisr3 wht problems were you facing due to ServiceLoader etc.?

@cowtowncoder I think this change should be reverted from 2.14.x releases at least, 3.x you can decide later.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Oct 17, 2022

This change will require the Aries SPIFly bundle and the jackson-core bundle will not resolve wherever Aries SPIFly bundle is missing.

The solution to that is to make the requirement optional, as has already been done with Woodstox. I'm sorry, I thought we already knew this?

@cowtowncoder
Copy link
Member

If there is more work to do, a new issue, PR is needed.
I do not typically re-open closed issues when they are referenced by release notes as that leads to confusion on specific versions issue relates to.

I am not currently working on any changes; and if some are needed for 2.14.0 it's important to get these done ASAP as the last planned release candidate (2.14.0-rc2) is out.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Oct 17, 2022

I am not currently working on any changes; and if some are needed for 2.14.0 it's important to get these done ASAP as the last planned release candidate (2.14.0-rc2) is out.

See #822.

@cowtowncoder
Copy link
Member

Excellent, thank you @chrisr3. This fell through the cracks on my side; I should have remembered the necessary fix.

@rakeshk15 WDYT? If this does not resolve the fundamental problem, yes, I can revert this from 2.14. However, not sure it should wait until 3.0 since future wrt major release is still but uncertain.

@rakeshk15
Copy link

I think what @chrisr3 did make sense, making the osgi.serviceloader.registrar requirement as optional.

This way the jackson-core bundle will always be resolved, if there is a osgi.serviceloader.registrar capability(Aries SPI Fly) available in OSGi runtime then the ServiceLoader mechanism will work otherwise it will be silently ignored.

@cowtowncoder
Copy link
Member

Ok good, thank you for confirming @rakeshk15.

@cowtowncoder
Copy link
Member

Unfortunately, had to revert due to #824. Not sure what would be the part forward here, but will not be happening for Jackson 2.14 as of now.

@chrisr3
Copy link
Contributor Author

chrisr3 commented Oct 24, 2022

Unfortunately, had to revert due to #824. Not sure what would be the part forward here, but will not be happening for Jackson 2.14 as of now.

I must say I'm surprised that Spring sees this warning because the @ServiceProvider annotation uses @Retention(CLASS), which should make it invisible.

@rakeshk15
Copy link

rakeshk15 commented Oct 24, 2022

Hi @chrisr3 the problem is due to the usage of Resolution enum in ServiceProvider annotation.

I think @cowtowncoder is right here to postpone it for post v2.14 releases, we don't know if someone else will face some other kind of issues if this fix is shipped with v2.14.

Other non intrusive fix(without the usage of any annotation lib etc.) would be to add the serviceloader.registrar OSGi Manifest headers manually inside pom (maven-bundle-plugin or bnd-maven-plugin(in pom or bnd file) configuration section).

But again, safer would be plan this for 2.14.x or even 2.15. I leave this decision to @cowtowncoder

@cowtowncoder
Copy link
Member

Thank you @rakeshk15. As to OSGi manifest: we are using Felix plug-in for OSGi bundle creation and I assume addition should be doable, but I would need someone else to provide that. If this could be done quickly I'd be ok in including such change still in 2.14.0 -- esp. if it gets in before 2.14.0-rc3 (at this point I think it is unfortunately necessary to do one more RC). I plan on closing issues today and if I have time do 2.14.0-rc3 release tomorrow.

@kriegfrj
Copy link

Other non intrusive fix(without the usage of any annotation lib etc.) would be to add the serviceloader.registrar OSGi Manifest headers manually inside pom (maven-bundle-plugin or bnd-maven-plugin(in pom or bnd file) configuration section).

This should work too and is probably the best short-term alternative.

@cowtowncoder
Copy link
Member

cowtowncoder commented Feb 23, 2023

PRs welcome for improvements; I don't quite know what to add.

But it would be great to improve the situation for upcoming Jackson 2.15 as well as Woodstox.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants