Consider supporting Guava 19 in grpc-api

[ejona86]

As requested by @adriancole at #5590 (comment) .

I worked to remove major ecosystem incompatibilities with Guava 20 starting in 2017 and claimed victory in 2018. See #4176 to follow the chain. Guava 20 was released in 2016, so I do have limited sympathy for libraries that remain incompatible, especially since every occurrence I've seen has included at least one @Beta API.

Supporting Guava 19 is a bit difficult from a testing perspective, unless we depend directly on Guava 19. If we continue compiling against Guava 20+ it's likely regressions will be introduced. Depending on Guava 19 directly would be unfortunate and hurts us getting to Guava 27, which we need to avoid Android dependency issues with listenablefuture and failureaccess.

@adriancole, I generally expect to know what libraries are incompatible with newer Guava versions and get some agreement from their maintainers they are working on the issue. That way I can track progress and know when we can use the newer Guava. What libraries are causing the trouble?

Also, note that we don't generally support users downgrading our published dependencies. We did "make it work" for a while for older Guava versions after we became aware it was wide-spread, but downgrading dependencies is not something that is acceptable without informing upstream.

I just tested and grpc-api is API-compatible with Guava 19. I think I'd take the approach of having our own Preconditions class in io.grpc that we use instead for the while. It can still call Guava's Preconditions, but it will ensure we only call overloads that are available in Guava 19. Note that this will cause more garbage creation, since the overloads were added in Guava 20 as a performance optimization. For cases where the garbage may be costly, we could implement the method ourselves. @adriancole, does that sound like a fine approach?

[codefromthecrypt]

thanks for opening the issue. to be clear the ideal would be to have zero guava in grpc-api, but I figured that would be shot down :)

FWIW many libraries have "solved guava 20plus" by repackaging it or rewriting it out of their stuff. since grpc is a low level api this makes the topic particularly sensitive. people above this should have choice in libraries, hence the suggestion to be flexible down here.

I'm mentioning because I recently had to rewrite HostAndPort over the guava 19 vs whenever fromHostText was removed. I suspect others are in similar positions, and maybe not all can redo their libraries to make a lowest common guava (often repackaging is that LCG). Remember that classpaths conflict with guava and even test tree conflicts can delete people's time.

Here in the api package, seems more than possible to do guava 19 as I noticed in the other issue where you pasted the classes used, hence the suggestion.

Thanks for opening the issue

[codefromthecrypt]

PS on your question about what is keeping people on older guava, I think this is just a reality of the job. Not all libraries frequently, and particular ecosystems such as hadoop are hard to move. People on top of those ecosystems get limited and if you have both in your classpath (even test) it hits.

Many libraries have a feature of not using guava anymore, or limiting it. For example, datastax cassandra driver "fixed" the problem by shading guava. You can imagine that pinning guava so low in the stack as the RPC layer will either unnaturally force all ecosystems to choose what's chosen here, or more realistically do what's been happening which is all tools vendor their own guava except grpc :P

[ejona86]

@adriancole, what library(s) require Guava 19 to function?

to be clear the ideal would be to have zero guava in grpc-api, but I figured that would be shot down :)

Dropping Guava from grpc-api isn't completely out-of-the-question. For your use-case alone, probably wouldn't happen, since there is too little "bang for buck." Too few users are like you as most by far will depend on grpc-core via transports. However, we have been actively considering it to ease combining grpc-context into grpc-api (and clean up the messes in #3522, #2847, #2727). That said, there are some other options available (like the "tricks" Guava did for listenablefuture). And there is no timeline for use figuring out which "way" we want to go.

I'm mentioning because I recently had to rewrite HostAndPort over the guava 19 vs whenever fromHostText was removed

HostAndPort is @Beta. It should not be used by libraries, unless those libraries shade Guava. This has always been the rule and it was clearly "advertised" by Guava. It also seems to be the greatest source of issues. I will agree it was way too easy to accidentally start using a @Beta API without realizing. That's why there is now a Guava Beta Checker, for libraries to verify proper usage.

Guava did previously have a policy where it reserved the right to remove stable APIs after 18 months. This caused trouble as well. However, that is no longer the case and Guava 21 is the last release to remove non-Beta APIs.

FWIW many libraries have "solved guava 20plus" by repackaging it or rewriting it out of their stuff

That's completely fine. As I mentioned, most issues seemed to be usages of @Beta APIs and shading or removing the dep are the only solutions to that.

You can imagine that pinning guava so low in the stack as the RPC layer will either unnaturally force all ecosystems to choose what's chosen here, or more realistically do what's been happening which is all tools vendor their own guava except grpc

Guava is forward-compatible starting with Guava 21. If libraries avoid @Beta APIs (and we complain loudly when they don't), then every part of the stack can depend on whatever version it wants. In the final application, use the latest of all dependent versions. Gradle does that automatically. If using Maven then use requireUpperBoundDeps.

If you want a @Beta API from a library, shade.

Shading Guava is "easy" and not a problem for large libraries/SDKs, as you can shade Guava once for the entire thing. But for small libraries it produces a lot of bloat throughout application dependency trees and is unacceptable at this scale for Android.

This section is identical for "grpc" as it is for "guava"; the same requirements hold, nothing is "special" about the "guava problem" other than it was a successful library with experimental APIs.

[codefromthecrypt]

Thanks for all the feedback, Eric. I noticed one question which is what libraries require older guava. The most recent time spent was in attempts to match spark. I think people have been pinged many times about this with issues noted by or to folks like @saturnism. I would be surprised if many people think guava conflicts do not occur regardless of if they think they should or not. Knowing they routinely occur, and the push to use grpc under everything.. it would seem reasonable to ponder whether it is congruent to pin a guava version into the API. agreed that maybe more eggs need to break first to make this worthwhile.

[ejona86]

@adriancole, hmmm... I can't really find much of a smoking gun for Spark. Yes, it seems to depend on an old version of Guava, but everything points to it shading that version and it has done so for many years.

https://search.maven.org/artifact/org.apache.spark/spark-core_2.11/2.4.1/jar ("provided", which is weird, but I assume the shading plugin did that)
https://issues.apache.org/jira/browse/SPARK-15384 (2016 evidence of shading)
https://issues.apache.org/jira/browse/SPARK-20756 (2017 evidence of (broken) shading, but it was a major bug and fixed)
http://mail-archives.apache.org/mod_mbox/spark-issues/201805.mbox/%[email protected]%3E (2018 evidence of shading)

Hmmm... It seems the problem may actually be triggered by hadoop. I do see a dependency on hadoop-client 2.6.5, which does not include the Guava fixes from 3.0.0.

That might also explain where the source of Guava came from to cause googleapis/google-cloud-java#4414 .

Note that in issue 4414 it seems to be causing a NoSuchMethodError for a new method. That means Guava was downgraded. Since the solution is userClassPathFirst, this seems like the classic "your class loader is exposing too much" which originally broke in servlet containers. This can/will break all libraries used by hadoop-client which includes Guava, log4j, slf4j, commons-logging, commons-lang, protobuf-java, jersey-json, and jakson-core.

While userClassPathFirst sort of helps, it is still a bit worrying. I may try to figure out how their dev process works to suggest they filter non-API classes, like done in servlet containers.

[ejona86]

And for reference, spark's class loading logic.

[codefromthecrypt]

thanks for the references, eric!

[ejona86]

The Java Servlet Specification discusses this some in SRV.9.7.2, but it is sort of hand-wavy.

The class loader that a container uses to load a servlet in a WAR must allow the
developer to load any resources contained in library JARs within the WAR
following normal J2SE semantics using getResource. As described in the J2EE
license agreement, servlet containers that are not part of a J2EE product should not
allow the application to override J2SE platform classes, such as those in the java.*
and javax.* namespaces, that J2SE does not allow to be modified. Also, servlet
containers that are part of a J2EE product should not allow the application to
override J2SE or J2EE platform classes, such as those in java.* and javax.*
namespaces, that either J2SE or J2EE do not allow to be modified. The container
should not allow applications to override or access the container’s implementation
classes. It is recommended also that the application class loader be implemented so
that classes and resources packaged within the WAR are loaded in preference to
classes and resources residing in container-wide library JARs.

I'm actually not sure what the last sentence implies, since it was already mentioned that the app shouldn't be able to access "container's implementation classes."

Tomcat's behavior: https://github.com/apache/tomcat/blob/f98e99d/java/org/apache/catalina/loader/WebappClassLoaderBase.java#L1251 . It first tries to load from the system class loader, then the webapp's class loader, then the parent class loader.

[pan3793]

IMHO, guava maybe the most conflict-prone package in Java world. As infra library, I will appreciate it if grpc and protobuf remove guava dependency, ideally they should be zero-dependencies.