grpc-java icon indicating copy to clipboard operation
grpc-java copied to clipboard

gRPC Java is not usable from Java 9 modules

Open aseovic opened this issue 6 years ago • 41 comments

Please answer these questions before submitting your issue.

What version of gRPC are you using?

1.6.1

What JVM are you using (java -version)?

java version "9"
Java(TM) SE Runtime Environment (build 9+181)
Java HotSpot(TM) 64-Bit Server VM (build 9+181, mixed mode)

What did you do?

Java 9 allows users to depend on older, non-modularized versions of the libraries by "converting" them to automatic modules. For example, when Maven dependencies on grpc are configured correctly, Java 9 allows me to do the following:

module myapp.foo {
    exports com.myapp.foo;
    requires grpc.core;
}

This allowed me to use classes from the grpc-core within my Java 9 module, but unfortunately it wouldn't compile:

ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.6.2:compile (default-compile) on project myapp.foo: Compilation failure: Compilation failure: 
[ERROR] the unnamed module reads package io.grpc from both grpc.core and grpc.context
[ERROR] module grpc.context reads package io.grpc from both grpc.core and grpc.context
[ERROR] module guava reads package io.grpc from both grpc.core and grpc.context
[ERROR] module error.prone.annotations reads package io.grpc from both grpc.core and grpc.context
[ERROR] module jsr305 reads package io.grpc from both grpc.core and grpc.context
[ERROR] module instrumentation.api reads package io.grpc from both grpc.core and grpc.context
[ERROR] module opencensus.api reads package io.grpc from both grpc.core and grpc.context
[ERROR] module grpc.core reads package io.grpc from both grpc.core and grpc.context

The issue is that Java 9 does not support split packages across modules and this is exactly what's happening here, as io.grpc package exists in both grpc-core and grpc-context, and to make things worse both grpc-core and grpc-stub have transitive dependency on grpc-context.

I've tried excluding grpc-context from both modules using Maven exclusions, which allowed me to compile successfully, as I don't have any direct dependencies on grpc-context. However, I was not able to run the test server, because of the missing Context class:

java.lang.NoClassDefFoundError: io/grpc/Context
        at io.grpc.internal.AbstractServerImplBuilder.build(AbstractServerImplBuilder.java:188)
        at io.grpc.testing.GrpcServerRule.before(GrpcServerRule.java:133)

There are several possible solutions, some better than the others:

  1. Merge classes from grpc-context into grpc-core and leave empty/dummy grpc-context module around for backwards compatibility (although most people probably do not depend on it directly).
  2. Do the same as above, but get rid of unnecessary grpc-context module.
  3. Rename the io.grpc package in grpc-context to io.grpc.context, which would eliminate split package issue, but would break existing code that uses classes from the current location.

In any case, I'm happy to help do the work, but someone will need to decide which approach to take.

aseovic avatar Sep 29 '17 11:09 aseovic

The reason for the split is to keep the dependencies down for grpc-context.

  1. Merging won't work. It would add the dependencies back in.
  2. Same reason.
  3. This is probably the most reasonable path, but that would break backwards compatibility. That makes it not so reasonable.

A package per jar is my own preference, but that ship has sailed.

How is Java 9 supposed to work with generated classes, that live in a parallel package hierarchy but different jars?

carl-mastrangelo avatar Oct 01 '17 23:10 carl-mastrangelo

cc @bogdandrutu as I remember some chatter about making context a top-level type (not pinned to grpc). Any related issue on that note can link here for another reason

codefromthecrypt avatar Oct 02 '17 05:10 codefromthecrypt

@adriancole the issue isn't that it's in the io.grpc realm, its that the same package is split over multiple jars. Making grpc-context contain io.grpc.context.Context would solve this.

carl-mastrangelo avatar Oct 02 '17 06:10 carl-mastrangelo

@adriancole https://github.com/adriancole the issue isn't that it's in the io.grpc realm, its that the same package is split over multiple jars. Making grpc-context contain io.grpc.context.Context would solve this.

right, but what I meant (and didn't say) is that if we are breaking the java package anyway, if there were a time to top-level the type, it would be now

codefromthecrypt avatar Oct 02 '17 06:10 codefromthecrypt

@carl-mastrangelo I can think of another option:

  1. Change the package name to io.grpc.context in grpc-context module. That will fix the split package issue going forward, but will break existing clients.

  2. Introduce io.grpc.Context and io.grpc.Deadline into grpc-core, which simply extend their io.grpc.context.* implementations in grpc-context. This would fix the clients that do not depend directly (and only) on grpc-context at the moment. It does imply, however, that io.grpc.context.Deadline could not be final, which it is at the moment.

Now, the bigger question is whether something like Context class even belongs in gRPC implementation, and I believe that's the 'top-leveling' @adriancole mentioned. It is a generally useful feature that most of us have written at least once, and would probably feel more 'at home' as java.util.concurrent.Context than anywhere else. The fact that the com.google.instrumentation:instrumentation-api depends on it also speaks volumes about where it does and does not belong...

Anyway, I've implemented a workaround that gets me unblocked for the time being by shading core, stub and context into a single JAR with a proper Java 9 module descriptor, but that's definitely not the right direction and I'm happy to help with both this issue and #3523 in any way I can (signed the CLA the other day, so we should be good to go from that perspective).

aseovic avatar Oct 02 '17 14:10 aseovic

This was discussed in #2847

zhangkun83 avatar Oct 03 '17 17:10 zhangkun83

Note: the generated code also can run afoul of Java 9 modules. If nothing else, the gRPC generated code is frequently in a separate JAR from the protobuf generated code, but the two exist in the same JAR.

ejona86 avatar Aug 13 '18 23:08 ejona86

So now the problem still exists in Java 11. Anyone can provide a solution(even a temporary one)? Module 'xxx' reads package 'io.grpc' from both 'grpc.core' and 'grpc.context'

UkonnRa avatar Oct 11 '18 02:10 UkonnRa

@CasterKKK, the only workaround I know is to repackage the two JARs into a single one as outlined in https://github.com/grpc/grpc-java/issues/2727#issuecomment-348209549

ejona86 avatar Oct 12 '18 23:10 ejona86

Same problem for our projects. Are there any plans to support JPMS?

marx-freedom avatar Oct 15 '18 08:10 marx-freedom

Users have requested support for Java 11 in Apache Beam and since it uses grpc it would need for grpc libraries to be compatible with Java 11. Hopefully this request helps increase the priority for fixing this.

lukecwik avatar Aug 08 '19 16:08 lukecwik

Is there any progress or ETA for this issue maybe?

lgajowy avatar Oct 16 '19 13:10 lgajowy

We'll be looking into options this quarter. Main choices are to try to reduce dependencies of grpc-api and then combine grpc-context with grpc-api or to do a trick like com.google.guava:listenablefuture's empty version 9999.0.

ejona86 avatar Oct 16 '19 14:10 ejona86

In the meantime, we have repackaged grpc-java (version 1.22.1) as a Java 9+ module as part of Helidon, in order to support our gRPC framework implementation, and have made it available on Maven Central.

This module basically merges grpc-core, grpc-context and grpc-stub into a single JAR, and adds necessary module_info.java to it.

@ejona86 This is not to say that we are not looking forward to a proper Java 9+ module support in grpc-java, which will allow us to get rid of this "necessary evil". But we needed something now...

aseovic avatar Oct 17 '19 03:10 aseovic

its now 2020/06/25, is there any progress here? Or everyone here still using ancient java 8 and planing to use until 2030+?

baha2046 avatar Jun 25 '20 09:06 baha2046

gRPC is compatible with Java 9+, and we continually test against Java 11. The problem here is for Java 9 modules. Helidon has a workaround by combining jars together, which does enable module support. And yes, we recognize that to be a workaround.

ejona86 avatar Jun 25 '20 15:06 ejona86

Does recognizing it as a work around also mean that the gRPC maintainers don't intend to solve this problem soon? It was mentioned it would be looked into the next quarter almost a year ago but doesn't seem to have any traction since. Is there any hope for a gRPC solution soon that isn't relying on a non-gRPC project deployed version that requires its own custom artifact management?

cjohnstoniv avatar Aug 19 '20 03:08 cjohnstoniv

I would very much prefer if the workaround we had to implement was part of grpc-java itself, instead of being "our" module that does nothing but repackages grpc-java artifacts. While it works fine for the end users, it makes working on Helidon gRPC support itself quite painful, and requires all kinds of IntelliJ acrobatics to get it to understand where different classes are coming from and stop highlighting gRPC class usage as compilation errors...

It may not be the ideal solution, but short of refactoring grpc-java to remove split packages, which would likely be a backwards incompatible change, I'm not sure what else can be done...

aseovic avatar Aug 19 '20 03:08 aseovic

I apologize if I am missing something but the problem I see as currently on the 1.30.2 version of gRPC is the split packages only exists between grpc-api and grpc-context on the io.grpc package. Is there any reason we can't put in a short-term, backwards compatible solution that involves possibly the following fairly ugly for a few version process:

  1. Copy all classes from grpc-context into grpc-api. It's already a dependency of the API and one could argue the Context is part the API so having a separate artifact of a few classes doesn't buy much. The classes are only copied not moved so they still exist in grpc-context.
  2. grpc-api removes dependency on grpc-context
  3. For N number of releases changes to the context classes (I would imagine fairly rare), should be applied in both grpc-context and grpc-api. This gives time for some planned movement off grpc-contexts as a sole-dependency supporting backwards compatibility. The classes yes will exist twice on the classpath, but SHOULD be the exact same so effectively nothing happens for existing Java 8 use cases. Java 11 apps can forcefully exclude grpc-context since they know its classes are in grpc-api now.
  4. Eventually grpc-context could remove all its classes and deploy as empty if needed and eventually (or immediately) never be deployed again

Apologize if this has already been attempted but this seems like an approach that allows modularity support fairly easily as provided by the gRPC project directly.

cjohnstoniv avatar Aug 19 '20 04:08 cjohnstoniv

@cjohnstoniv What you are proposing is effectively the same as option 1 (short term) and option 2 (long term), which were rejected by @carl-mastrangelo because "they would bring dependencies back in".

The problem with leaving classes in both places is that you make grpc-context and grpc-api mutually exclusive: as soon as something brings the dependency on grpc-context back in (such as instrumentation-api, for example), you end up with split packages again :-(

aseovic avatar Aug 19 '20 04:08 aseovic

Is it not possible to also migrate future versions of instrumentation-api to depend on grpc-api instead of grpc-context? instrumentation-api would need to move to grpc-api anyway to support anything that depends on it for modularity.

What about "they would bring dependencies back in" is more powerful than gRPC being the only non-modularity supportable dependency for several projects? Maybe I'm missing something about what "they would bring dependencies back in" means.

I don't think grpc-context re-appearing on the classpath through some other dependency transitively is the end of the world. At least in this case the developer of the library/application can fully control just excluding grpc-context from their tree. Also anything that depends on grpc-context directly would need to fix itself anyway for the library/app to be capable of using modularity. This also wouldn't break backwards compatibility if it occurred because it currently doesn't work today.

cjohnstoniv avatar Aug 19 '20 04:08 cjohnstoniv

@cjohnstoniv, it is nowhere near as simple as you think. It is a land of subtle issues and hard trade-offs.

Let's not forget that this is a self-inflicted Java module issue. Java modules enforced new restrictions on existing code. It's also made more difficult by Maven's weak package management. We are trying to shoe-horn an existing system into new constraints without any tools to support such a transition, while not taking anything away in the process. If Java supported type aliases or if Maven supported marking "conflicting" packages this would be a lot easier.

The simplest change is to move grpc-context into grpc-api such that grpc-context is empty and depends on grpc-api. This clearly throws dependency-sensitive grpc-context users "under the bus" as the jar size would change from ~30K to ~280K and add dependencies. It would also make grpc-api harder to maintain because we would start avoiding Guava (probably shading parts of Guava, but then we have to be sensitive to how much of Guava will be copied with each class we use). So while being simplest, this approach actually has the longest-term ramifications.

I don't think grpc-context re-appearing on the classpath through some other dependency transitively is the end of the world.

It is bad. Really bad. Users aren't able to debug that. The results are effectively non-deterministic. You get really weird errors that appear to have nothing to do with the actual problem. We see something similar already when Maven downgrades packages; which users can't deal with it. But in that case there is requireUpperBoundDeps which helps a lot. With this there is no such checker available and it would require a package format change to add a checker. We have to avoid this.

But maybe with a "trick" we could use the general approach but avoid the duplication on the classpath. ~~Hacks~~ Tricks have been considered; a large variety of ~~hacks~~ tricks. But it's really easy to miss issues that will crop up. And for the issues you find you have to consider cost/benefit while being mostly blind (that is to say, we need to weigh options on cost/benefit, but we don't actually know the cost/benefit; we have to guess).

We did work on this a year ago, but it sort of blew up in our face as we discovered issues with the approaches that made the choice more complex and caused us to consider new options to workaround those issues. We have actually come back to it again this quarter and have made good progress. But it is clear that every solution hurts someone.

ejona86 avatar Aug 20 '20 16:08 ejona86

@ejona86 I really appreciate the update on information about this effort. I certainly understand there isn't a way to do this without trading off and every solution likely hurts someone.

If we wanted to not throw the dependency-sensitive users "under the bus", could we not just add dependency exclusions for grpc-api dependencies that grpc-context does not need in the grpc-context artifact? Wouldn't this reduce the dependencies of grpc-context without comprising anything also with grpc-api? There might be some maven dependency resolution corner case impacted here, but as you said every solution hurts someone. When it comes to Maven, some of these dependency resolution issues are really just the "way of the road". At any point in time a dependency (including transitive) can add a new dependency throws off your entire resolution tree, this really wouldn't be much different than that scenario the way I see it.

cjohnstoniv avatar Aug 22 '20 23:08 cjohnstoniv

could we not just add dependency exclusions for grpc-api dependencies that grpc-context does not need in the grpc-context artifact

That's a good idea, and isn't on the list. Eric runs off to add it to the internal option list.

It would still increase the size of the jar being used from ~30k to ~280k, but avoids long-term maintenance issues. It does export grpc-context users to dangers if they use other grpc-api APIs, as those implementations may be broken without their necessary dependencies. But that may be a worth-while tradeoff for them.

At any point in time a dependency (including transitive) can add a new dependency throws off your entire resolution tree, this really wouldn't be much different than that scenario the way I see it.

Yes, but since users aren't commonly able to solve the problem on their own, we do get to consider the clarity of errors triggered since we may need to provide "remote debugging" support to tell users where their dependency tree is broken.

ejona86 avatar Aug 24 '20 15:08 ejona86

For those looking for a quick and convenient workaround, I've created a temporary library that solves the split package problem by merging grpc-api and grpc-context into a single jar. It's available in maven central. The repository is here, and the usage is below (gradle kotlin DSL) . First exclude your dependency of "grpc-context" and "grpc-api", then add this library as a dependency :

repositories {
    mavenCentral()
}

dependencies {
    implementation("org.rationalityfrontline.workaround:grpc-api:1.54.0")
}

configurations.all {
    exclude(group = "io.grpc", module = "grpc-context")
    exclude(group = "io.grpc", module = "grpc-api")
}

RationalityFrontline avatar Oct 15 '20 03:10 RationalityFrontline

will this ever be fixed?

rnayabed avatar Oct 29 '20 18:10 rnayabed

Not sure if I'm missing something here, but isn't changing the major version the point of backwardly incompatible API changes?!

  • v1 branch: keep the status quo for the time being but deprecate,
  • v2 branch: refactor to io.grpc.context.* and implement module-info.java.

~Progress on this sooner rather than later would have avoided potential impact to things like OpenTelemetry that is now bound to io.grpc.Context, however that's still in pre-release (0.10.0 at time of writing) so any package changes could be accommodated.~ Update: Looks like I jumped the gun on OpenTelemetry, 0.10.0 has specifically moved away from gRPC context due to that exact reason.

dansiviter avatar Nov 10 '20 09:11 dansiviter

@dansiviter

Looks like I jumped the gun on OpenTelemetry, 0.10.0 has specifically moved away from gRPC context due to that exact reason.

No, that's not true. They had to make their own because their spec said they had to if there was not a commonly agreed upon one for the language. And even without this issue, there are many competing contexts in Java.

We cannot accept a v2 major version for this change. And if we did that would cascade downstream to all APIs that use it. If we introduced io.grpc.context.* it would just be within our current 1.x branch, since they have different names.

ejona86 avatar Nov 12 '20 21:11 ejona86

@ejona86 Although a breaking change is diametrically against keeping a major version number, if we can move from this impass with a package change in 1.x I think that will unblock a lot of developers. Any idea when this may happen?

dansiviter avatar Nov 12 '20 21:11 dansiviter

@ejona86 Any update on this, is this the best issue to follow?

klaraward avatar Jun 29 '21 09:06 klaraward