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

FlowAdapters splits the org.reactivestreams package #424

Closed
jroper opened this issue Feb 21, 2018 · 11 comments

Comments

@jroper
Copy link
Contributor

commented Feb 21, 2018

It is impossible to consume the reactive streams flow adapters in JDK9 as a module since it splits the org.reactivestreams package across itself and the org.reactivestreams module (which it depends on). Here's an example error message from javac when you try to use it:

error: module org.reactivestreams.tck reads package org.reactivestreams from both org.reactivestreams.flowadapters and org.reactivestreams

JDK9 does not allow packages split between modules. FlowAdapters must be moved to its own package if its to be in its own artifact.

@jroper jroper changed the title Reactive streams flow adapters splits the org.reactivestreams package FlowAdapters splits the org.reactivestreams package Feb 21, 2018

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2018

I guess we can work around it by having the latest version of the flow-adapters jar but the non-automatic-module-name version of the API-jar? Or?

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

@reactive-streams/contributors Any thoughts or suggestions here?

@akarnokd

This comment has been minimized.

Copy link
Contributor

commented Mar 26, 2018

No clue. I have no experience with Java 9 modules but fixing this sounds like we have to break the structure of library code already released.

@jroper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

We could just include the adapters in the main reactive streams jar. It won't cause any problems for users of JDK8 and earlier, since they just won't use them.

We could also use MR jars to make org.reactivestreams.* extend juc.Flow.*, however the JDK maintainers have explicitly stated that that's not an intended use case for MR jars, so we'd be on shaky ground, particularly when it comes to whether other tooling (IDEs, compilers for other JVM languages) supported it.

@ktoso

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

We could just include the adapters in the main reactive streams jar. It won't cause any problems for users of JDK8 and earlier, since they just won't use them.

Yes, this works;

Akka has been doing this since JDK9 was released: akka/akka@349a5f7 ( akka-stream/src/main/java-jdk9-only/akka/stream/javadsl/JavaFlowSupport.java) The existence of such JDK9-refering classes in a JAR does not cause problems for JDK8 users.

The classes can remain "where they are" package-wise then, so the change is source as well as binary compatible.

It also means deprecating the adapter artifact as it would not be intended to be used. It may happen that someone ends up with both artifacts on the classpath... "new" RS that contains adapters, and "old RS adapters", however since the classes are to be exactly the same that's managable risk... and should not really happen one would hope (though stating the fact that that's the risk/tradeoff we pick when we do this)

MR jars

Yeah it's not really their purpose. I would leave them out of the game -- we did investigate this quite deeply when doing the decision for the adapters in Akka Streams, deciding that it'd be abusing the mechanism, so we decided to stick to "plain boring but works" including of the JDK9 classes in artifact as usual. This makes the build a bit of a pain, but it is doable.

Is there any tooling that we should have been using to verify this problem?

Nope; no tools exist for this. The module dance 💃 is something library maintainers have to plan rather much ahead and precise... Too bad we missed it here, but it's not not-fixable thankfully this time around.

@jroper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 27, 2018

There will be tooling some day, but it's far too soon, particularly as no one seems to have any interest in upgrading JDKs until JDK11 at a minimum. Even then, the module system is incompatible with much of the Java ecosystem, due to problems like this, and isn't even compatible with current practices for unit testing (where you put unit tests in the same package as the code you're testing), without major tooling to work around that (you can do it by passing several hundred characters worth of command line options to both the compiler and java itself, these command line options allow you to patch a module with classes from another module, but it's very verbose and not at all practical to maintain these command line arguments manually, since you have to do it not just in your build tool, but also in your IDE if you want to run/debug tests in the IDE). So there's little incentive for anyone to create tooling to verify that modules work correctly any time in the near future.

@akarnokd

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

We could also use MR jars to make org.reactivestreams.* extend juc.Flow.*

This doesn't work in both directions: #394.

include the adapters in the main reactive streams jar

Technically possible, but some developers like to dig around and will attempt to use them from Java 6..8 anyways, then complain about it doesn't work.

@ktoso

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

include the adapters in the main reactive streams jar

Technically possible, but some developers like to dig around and will attempt to use them from Java 6..8 anyways, then complain about it doesn't work.

I find that counter argument not convincing -- "someone complains" is weaker than "it is not possible to use" which we are now facing due to the module situation :)

@viktorklang

This comment has been minimized.

Copy link
Contributor

commented Mar 27, 2018

I think a viable solution is to move the adapters to the main jar, and then clearly JavaDoc that they require JDK9+ This wouldn't break any app since duplicates are ignored (if old adapters jar is still on the classpath), and we can just decide not to release the adapters jar anymore?

@jroper

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2018

How will anyone try to use them from Java 6..8? Before they try to use them, they'll need to have a use case to use them. A use case to use them means they need some other API that accepts/produces juc.Flow that they want to use. But, since they are on Java 6..8, that other API also won't work, they won't be able to compile against it, let alone load it at runtime, since juc.Flow doesn't exist in Java 6..8.

And if after explaining that, they still find a reason to complain anyway, what's wrong with that? They're complaining about something that anyone with the smallest amount of common sense would understand is logically impossible, it's not a matter of opinion about why they won't work and why we can't make them work on Java 6..8, it's a fact, and a refusal to accept that is irrational. I don't think we should optimise our APIs to ensure that irrational people won't complain about them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.