reactor-core 3.1.0 requires Kotlin's StdLib and jsr305 in OSGi environments

[magnet]

Expected behavior

reactor-core wouldn't need Kotlin. Not sure about JSR305, these are runtime annotations.

Actual behavior

The reactor-core bundle cannot be resolved in an OSGi framework unless kotlin-stdlib-osgi and jsr305 are resolved.

Reactor Core version

3.1.0

Possible solutions

For Kotlin

• Put Kotlin extensions in a separate artifact (cleaner solution, Kotlin users could depend on that artifact instead of reactor-core)
• Make kotlin and kotlin.* optional in the Import-Package manifest entry (this works only as long as Java code never calls Kotlin code, since we can guess Kotlin user code will already depend on Kotlin's stdlib)

For JSR305

Marking these annotations as optional seems risky. Those annotations used have a runtime retention, and are used by reactor-core's @NotNull, @NonNullAPI and @Nullable annotations which themselves have a runtime retention. Unless reactor-core's annotations are made compile-retention, someone could reflexively find some parameter annotated with one of them and that would trigger the class loading of its runtime annotations (from jsr305) -- which would lead to a ClassNotFoundException. However, a bunch of these annotations are in javax.annotation which is a split package; importing it would be a headache as well :)

Hence it would seem to me making reactor-cores's annotations compile-time could be the better solution, and tools doing static analysis should work fine because afaik they do the bytecode loading themselves.

[sdeleuze]

For Kotlin, I think your second proposal makes sense for both Reactor strategy regarding to Kotlin, it is a builtin but optional Kotlin support, and everything is done to not call Kotlin when it is not present. Notice that Kotlin dependency is declared as optional in Gradle build, so while I have no OSGI knowldge, it is probably a good idea to apply these semantics to OSGI as well.

For JSR 305 meta-annotations, there are designed to be used reflectively only by tooling, so no need to explicitly declare this dependency (not even as optional) unless OSGI has some specific behavior. The only case when JSR 305 JAR is needed is for Java Reactor libraries in order to avoid a compile time warning, projects consuming Reactor libraries should not require such library.

[magnet]

@sdeleuze Making Kotlin's stdlib optional is an adequate solution in OSGi from a technical point of view, and I think Kotlin is great, but from a modularity point of view I feel it would be just as easy to have these extensions in a separate artifact.

About JSR 305, the problem is with reactor-core's own annotations @NotNull, @NonNullAPI and @Nullable having a runtime retention. It drags the whole thing, while I believe a compile time annotation would work just as fine for tooling. As soon as the retention scope is changed, I believe bnd, the tool inspecting the bytecode to generate the OSGi manifest, will stop considering those are runtime dependencies.

If you like I can provide a pull request with the changes; I'd also like to re-evaluate the need to have com.google as an optional dependency, I am not sure Guava is used at all (anymore) by reactor-core. Same goes with RxJava, which is imported optionally but I believe not used.

[sdeleuze]

@magnet Do we have a way to customize the generated OSGI manifest to avoid exposing jsr305.jar as a dependency?

[magnet]

@sdeleuze It would lead to a malformed module (as in, wrong metadata) to forcefully hide this if it is required on those annotations and keep them as runtime annotations. For instance, calling getAnnotatations() on a parameter annotated with @Nullable would load Nullable.class, which would then load the meta annotation. The same problem exists outside of OSGi, with plain Java, if someone does that, and it's an issue right now :-)

If I have that code

 public void foo(@Nullable @MyOwnAnnotation bar) { }

and if I try to find @MyOwnAnnotation, if I don't have jsr305 on my classpath (or installed as a module if I'm running OSGi), it is going to fail.

Most, if not all, null-analysis tools have their own code to load bytecode (e.g through ASM) and work just fine with compile-retention annotations

[sdeleuze]

Based on my tests, I don't think it is going to fail.

These annotations have runtime retention on purpose since they are used by IDE and Kotlin to infer API null-safety based on Reactor JARs (not source code), turning them to compile-time is just impossible. JSR 305 is not perfect but current arrangement with Reactor annotations meta-annotated with JSR 305 is the best outcome we could achieve for now. Be aware that I am trying to see if we could use proper dependency which would not using javax.annotation package, see this discussion for more details.

[magnet]

@sdeleuze You are right, I just tested this as well and runtime annotations not found on the classpath are simply ignored. Supposedly it is described there in JLS 13.5.7: "... removing annotations has no effect on the correct linkage of the binary representations of programs in the Java programming language."

That makes the difference between class (available in bytecode) and runtime retention (available in bytecode and loaded by the JVM) quite narrow.

I still believe tools would work fine with class retention annotation (for some reason I remembered it as "compile" which was confusing; with class retention the annotations remain in the bytecode/JARs unlike with source retention, which is only kept in source code!). Have you tried changing that retention to class and see if your tools and Kotlin compiler handle it? I believe they should.

It should be possible to forcefully remove the javax.annotation imports by setting in the bnd Import-Package directive: !javax.annotation,!javax.annotation.*. Because not found runtime annotations are silently ignored, it would result in the same behavior as a class time retention.

[sdeleuze]

Indeed good point, Kotlin compiler will recognize those annotations with default class retention, but won't be available with reflection. I am not aware of such usage on Reactor side, but there are on Spring side and I really would like to keep both side consistent to avoid unwanted side effects and subtle differences.

Another reason is that we are working with Jetbrains to try to provide an alternative to FindBugs jsr305.jar with dedicated package which will be not an issue to expose as an optional dependency. So I tend to think this is not the good timing for such change.

For now, I think customizing bnd configuration as suggested is a more reasonable option.

[magnet]

Looks good to me. If this is indeed used to see constraints within Spring, it makes sense. I also dislike having javax.validation.constraints.NotNull for runtime and another annotation for static analysis.

Changing the package and having a clean, OSGi+JPMS-compliant packaging (e.g no split packages, semantic versioning) would definitely be a great improvement. It would be great to have that available in a JVM-neutral place as well :-).

[Azbesciak]

Failed to start bundle io.projectreactor.reactor-core-3.1.2.RELEASE, exception Could not resolve module: io.projectreactor.reactor-core [50]
  Unresolved requirement: Import-Package: kotlin; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.collections; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.jvm; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.jvm.functions; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.jvm.internal; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.jvm.internal.markers; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.reflect; resolution:="optional"
  Unresolved requirement: Import-Package: kotlin.sequences; resolution:="optional"
  Unresolved requirement: Import-Package: sun.misc

bnd.bnd:

-runbundles: \
...
	org.reactivestreams.reactive-streams;version='[1.0.1,1.0.2)',\
	io.projectreactor.reactor-core;version='[3.1.2,3.2)',\
...
-buildpath:\
...
	io.projectreactor.reactor-core;version='[3.1.2, 3.2)',\
	org.reactivestreams.reactive-streams;version='[1.0.1,1.0.2)',\
...

maven deps:

io.projectreactor:reactor-core:3.1.2.RELEASE
org.reactivestreams:reactive-streams:1.0.1

any deps from kotlin, so i am asking is it an error in my build bnd or it is not so optional :)?

EDIT:
Deps are trully optional, however... sun.misc not.
Connected with ReactiveX/RxJava#3516, this solved my problem.