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

Regarding splitting away ListenableFuture from Guava #3320

Closed
jbduncan opened this issue Nov 20, 2018 · 37 comments
Closed

Regarding splitting away ListenableFuture from Guava #3320

jbduncan opened this issue Nov 20, 2018 · 37 comments

Comments

@jbduncan
Copy link
Contributor

This issue is a follow-on from the discussion started by @jodastephen at #3302 (comment) about the recent introduction of the com.google.guava:listenablefuture and com.google.guava:failureaccess dependencies.

To summarise, when @cpovirk is back from leave, I'd love to hear his reasoning for the introduction of the two new artifacts, and to discuss things (whether publicly or privately) with both @cpovirk and @jodastephen to see if it would be possible to achieve what the Android Support library authors would like (being able to use ListenableFuture in the Android SDK, IIUC) whilst satisfying @jodastephen and myself (keeping Guava as one artifact, or as close to this as possible).

@jbduncan jbduncan changed the title Regarding splitting away of ListenableFuture from Guava Regarding splitting away ListenableFuture from Guava Nov 20, 2018
@jodastephen
Copy link
Contributor

As it stands, I have no desire to upgrade Guava beyond v26, which could be a problem if there is a security issue.

In OpenGamma Strata, we have to list all dependencies (including transient), so the new setup (particularly the v9999 hack) is problematic (just try explaining it!).

In Joda-Collect, this change triples the number of dependencies. Things are not much different in Joda-Beans and Joda-Convert.

My proposal is that Guava v28-jre has no dependencies, but v28-android does. From a quick look at the code, I don't see why that wouldn't work.

@sparty02
Copy link

Maybe consider using the shade plugin to inline the separate bits in guava if they really should be modular?

@jbduncan
Copy link
Contributor Author

Hey @JakeWharton, I saw that you thumbed down @jodastephen's earlier comment, so I wondered if you would be willing to explain to us your reservation(s) with his idea? :)

@qualidafial
Copy link

My perspective using Guava in both open source and private projects:

  • Virtually every Java project uses Guava.
  • Many projects don't keep their Guava dependencies up to date--even open source maintainers.
  • Guava removes API a few releases after deprecation.

Taken together, these facts frequently lead to impasse for project maintainers. One cannot update Guava until dependencies that use Guava are updated. You are constrained to the schedule of your worst-maintained dependency.

One workaround we have resorted to at $EMPLOYER is to shade Guava into libraries which use it purely internally. However with the recent split into multiple jars, it is difficult to get the shading right. Did I remember to promote transitive dependencies? Should any of those transitive deps get shaded too?

Where I work, this has forced us to institute a rule that apps may use Guava as a dependency, but libraries may not depend on Guava. An exception is made for libraries that integrate Guava with something else.

For what it's worth.

@jbduncan
Copy link
Contributor Author

  • Guava removes API a few releases after deprecation.

Not any more IIRC. :)

@qualidafial
Copy link

qualidafial commented Nov 21, 2018

@jbduncan Per the Guava 26.0 release notes, CharMatcher static constants were removed in favor of static factory methods, and some Futures static methods were removed.

These APIs were marked as @Beta, so were eligible for removal at any time. In practice though, people use those APIs anyway, even in libraries--often without realizing. The end result is the same.

@jbduncan
Copy link
Contributor Author

@qualidafial Ah, yes, you're right about CharMatcher and other beta APIs! I forgot about that catch. Thanks for correcting me on that.

@cgdecker
Copy link
Member

@jbduncan @qualidafial FYI, the CharMatcher constants were an exception we called out explicitly when we announced the changes in regards to the new deprecation/removal policy. They'd been deprecated for quite a long time, and not removing them would have made the whole reason for deprecating them and replacing them with static methods moot (that is, not eagerly loading various classes you were never going to need when loading CharMatcher because they were needed to initialize the static fields).

@qualidafial
Copy link

@cgdecker Could you point me to where that policy is documented? I'm looking at https://github.com/google/guava/wiki/ReleasePolicy and https://github.com/google/guava/wiki/Compatibility but I don't see anything that lays out under what conditions deprecated code may / will be removed.

@ogregoire
Copy link

ogregoire commented Nov 27, 2018

@qualidafial Please see here: https://groups.google.com/forum/#!topic/guava-discuss/rX-QXo-67ZU

  • Even if an API is already @Deprecated, we no longer plan to delete it (unless it's @Beta).
    Technically, there is one exception: We deprecated the CharMatcher constants back when they were @Beta. Then we removed @Beta from the class. Because these methods were never present as non-@Deprecated, non-@Beta APIs, and because they are expensive to initialize on Android, we'll still remove them.

@talios
Copy link

talios commented Nov 27, 2018

More precisely, we won't make any kind of binary-incompatible change (again, except to @beta APIs). For brevity, I'll say just "remove" in this doc. We use the more precise phrasing in the official policy.

A small bit of pedantry here maybe - but surely removing classes, causing a new dependency to be introduced into the usage ecosystem, is a binary-incompatible change.

One can't simply just drop in this new version of Guava and have things work without additional changes.

@cgdecker
Copy link
Member

@talios No, I don't think that's a binary incompatible change at all. Whether the classes you need come from one jar or two seems immaterial in terms of binary compatibility.

@talios
Copy link

talios commented Nov 28, 2018

@cgdecker as far as the OSGi manifest is concerned, that internal package that we've removed was part of the public/exposed contract of Guava, you can't just swap guava.jar versions and have things still work - you'd also have to deploy the additional bundle to satisfy its contract.

That may not be "binary compatible" to some - but in an OSGi context - I think it sure is.

@kashike
Copy link

kashike commented Dec 11, 2018

It is also worth noting that both guava and failureaccess both have the com.google.common.util.concurrent.internal package, which is a no-no with Java 9+ and modules.

@cpovirk
Copy link
Member

cpovirk commented Jan 4, 2019

1. It's entirely possible that this was the wrong decision. I can't remember feeling so uncomfortable with a big decision in recent years. I still think it's narrowly the right decision -- and if it's not, that's on me -- but there many tradeoffs and unknowns, and it's possible that we'll never be able to convincingly prove whether it was right or wrong. (I know that others are already convinced, so I'll try to make a case for this below. I have been reading the posts here with interest.)

2. ...unless, of course, we keep finding outright bugs. Then it will clearly be the wrong decision. I'm sorry for the trouble with OSGi and the duplicate com.google.common.util.concurrent.internal classes. I thought I had specifically tested for both of these, but clearly I got something wrong. If I had expected there to be a large chance of such problems, then I wouldn't have made this change.

3. Most developers encounter only one side of the tradeoffs. The problems that we aimed to solve here are mostly large-system problems: My app doesn't respond promptly to user cancellation requests, or its servers flood one another with badly scheduled retries, or it's generally unreliable because its various concurrency libraries respond slightly differently to failures. These are the kinds of problems that we have seen ListenableFuture help with -- not because ListenableFuture is complicated, of course, but because it's a common building block that behaves predictably and has libraries built atop it. And if you've got 100k lines of code and 50 deps, you're probably willing to add a couple more deps to get it, even if your build setup requires you to list them explicitly (and we've seen that inside Google, too...). On the other hand, most libraries and apps are nowhere near that size -- and if they were, they'd be making their users' lives worse. For them, extra dependencies -- extra configuration, extra jars to download, extra failure cases to be aware of -- have a larger cost. And the benefits are smaller, likely even zero, since most people aren't mixing multiple concurrency libraries. The new artifacts are a conscious attempt to help some developers with large-system problems, even though it has costs that fall primarily on the rest of developers. The outcome I'm envisioning here is that some developers' lives get more complicated, but some of the web and phone apps we use become less buggy over time. Will it work out that way? Well, see #1 and #2 😕

@jodastephen
Copy link
Contributor

Thank you for responding. Unfortunately, it doesn't really provide a route forward for those of us that care about dependencies (and find the 9999 hack completely unacceptable). As it stands, we are stuck on v26.

And the benefits are smaller, likely even zero

No, the benefits are not zero, they are negative.

I have to stress again, dependency count is really important for libraries, and especially low level ones because each dependency added has a multiplier effect up the stack. Ultimately, each dependency that a library has increases the chance of jar hell for someone in the large-systems you discuss. Developers like me are trying to reduce and minimize the pain for large-systems, changes like this really don't help.

I'll ask again, why not make v28-jre a single jar, while the android release is split? The jre release is Java 8+, where CompletableFuture provides what many would argue is a superset of the benefits of ListenableFuture (thus making it a complete irrelevance to break out ListenableFuture into a separate jar).

However this thread ends, it does feel like Guava has damaged the trust that allowed it to be effectively an extension to the JDK.

@cpovirk
Copy link
Member

cpovirk commented Jan 25, 2019

Sorry for the very slow responses. I've been OOO a bunch, but that should be winding down.

If we make 28.0-jre a single jar, its copy of InternalFutureFailureAccess may conflict (in the JPMS/OSGi sense, plus the case of the normal Android toolchain, which looks for overlapping classes) with a copy from failureaccess if a user also depends on that. We could employ the 9999 hack again, but I'd like to understand if that's better or worse from your perspective.

Your larger point about trust deserves a further response, which I'll try not to keep you waiting another week and a half for.

@talios
Copy link

talios commented Jan 28, 2019 via email

@cpovirk
Copy link
Member

cpovirk commented Feb 4, 2019

@talios: Hmm, true, failureaccess is likely to be used only from Android libraries. It's possible that other libraries could use it (especially cross-platform ones), but maybe the risk is worth taking. If we went back to a single jar for the -jre Guava but with another 9999 hack for the failureaccess jar, would that work out OK for you?

(Maybe the 9999 hack would even permit a single jar for the -android Guava? I need to run right now, but I should think about this more.)

@cpovirk
Copy link
Member

cpovirk commented Feb 4, 2019

To the general point about jar hell: More jars is definitely worse. I am hopeful that have at least not increased the amount of "classic" jar hell, since our new jars each have only one "real" version. So, users shouldn't encounter a situation in which one dep needs version X and the other needs version Y. They may still encounter a situation in which they need to exclude one entirely. I'm hoping that that proves to be an acceptable risk in comparison to the benefits of a common ListenableFuture type. But it's fair to say that I didn't expect failureaccess -- which provides a small fraction of the benefit of our changes -- to generate such a large share of user concerns.

@jodastephen
Copy link
Contributor

I think the best position is to go back to how it was in v26.0-jre and pretend v27-jre didn't happen. Obviously this does create the possibility of jar hell around com.google.guava:failureaccess. I'm not clear that a v9999 of that library would really help that (as nothing would depend on it) but maybe it would be a useful marker.

I'm not sure there is a second best option. The listenablefuture 9999 hack is the part I most object to, but the failureaccess isn't much better as it only contains classes in an "internal" package (thus should be able to be freely moved and deleted). In fact, AFAICT the two failureaccess classes didn't exist in v26.0-jre, so its very unclear why they were even added.

On Android its perhaps too late. But if not, one option might be to have three versions of Guava (and no additional libraries):

  • jre
  • android
  • android-minimal (instead of listenablefuture and failureaccess, and only containing the 3 classes)

With this approach, libraries would depend on one version of Guava and nothing else. And all three would have no dependencies.

(To end up lower in Maven's version ordering, the third jar can't actually be named "android-minimal", it would have to be "andrmin" or maybe "accessfuture", see org.apache.maven.artifact.versioning.ComparableVersion)

@albertotn
Copy link

Came late to the party, following swagger-api/swagger-core#3122

Use case

I've updated swagger-core for my project and found my server .war archive much bigger. Why? Because there are more dependencies and more importantly.. there is a guava-android, why?

Expectation

As developer and maintainer, my expectation is to have a versioning model that is stable. So changing name is fine, but not too much often, please!

Solution

I agree with @jodastephen in particular:

I have to stress again, dependency count is really important for libraries, and especially low level ones because each dependency added has a multiplier effect up the stack. Ultimately, each dependency that a library has increases the chance of jar hell for someone in the large-systems you discuss. Developers like me are trying to reduce and minimize the pain for large-systems, changes like this really don't help.

I want to stress how much confusion is to have a new dependency that is called "android" pop-up on a server side project.

@jbduncan
Copy link
Contributor Author

jbduncan commented Mar 1, 2019

@albertotn Thanks for chiming in! I'm not entirely sure this is the right issue to discuss guava-android though, as in this issue we're discussing the merits of the newer listenablefuture and failureaccess artifacts.

But I'm sure that I've missed something. :)

Could you clarify for me why you think this issue is related to your issue where you're seeing guava-android being automatically imported into a JRE-based project?

@albertotn
Copy link

From an outside perspective is only a matter of names :D spot an android named jar inside a server side war starts some questions on what is going on (at least for me)

@lowasser
Copy link
Contributor

lowasser commented Mar 1, 2019 via email

@jbduncan
Copy link
Contributor Author

jbduncan commented Mar 1, 2019

@albertotn I agree with @lowasser; I thus wonder if there's a misunderstanding about what this particular issue is discussing.

To give some context: a good while ago, the Guava team decided to release a version of Guava called guava-android, the code of which is slightly different to normal Guava such that it can be used by users on Android and Java 7.

By comparison, this issue is discussing something quite different where more recently the Guava team split away the ListenableFuture class into its own artifact, which @jodastephen and I are concerned about because, the way the Guava team it, there are now two new artifacts rather than one - the expected listenablefuture artifact as well as a very strange-looking one called failureaccess - and both of these artifacts are now dependencies of Guava, so it's not so easy for developers to just import Guava as-is if they don't use a package manager like Maven or Gradle, and in @jodastephen's case it makes it much harder to explain to the users of his company's product what each dependency does, because e.g. its not clear even to us Guava users what failureaccess actually does!

So the way I see it, I agree with @lowasser in that your problem looks different to what this issue is discussing, in that your project probably has a dependency which itself imports guava-android rather than normal Guava.

If I've understood your problem correctly, may I suggest that you open a new issue or raise a question on StackOverflow to discuss it further? :)

@jbduncan
Copy link
Contributor Author

jbduncan commented Mar 1, 2019

There's also the problem that increasing Guava's dependencies like this increases the chance of "JAR hell" happening for Guava users, but I'm relatively inexperienced in this topic, so I don't know enough to comment about it.

@rhuffman
Copy link

rhuffman commented Apr 8, 2019

I'm also late to the party, but I would like to chime in with my $0.02.

If I include failureaccess 27.1 in my (server side) project, I must also depend on guava-parent 26.0-android. That is definitely weird. It will cost me time to convince our compliance team this is necessary, then it will probably cost multiple people time in the future as they try to track down why an Android artifact is being pulled into their projects.

If this discussion lands on the side of keeping the separate artifacts (which I hope does not happen), can you at least do something about naming the parent poms?

For now, I'm sticking with 26.0 and hoping that does not bite me in the future.

@cpovirk
Copy link
Member

cpovirk commented Apr 12, 2019

Sorry about the parent-pom issue. I found that we needed at least some of the configuration from the parent pom, and I didn't realize that using it (rather than copying the configuration) would cause people compliance issues.

We still have things to figure out here. It does sound like we could probably just release another version of each of our "one-version-only" artifacts and encourage people to do that, but maybe we'll be able to do something larger, too.

@jbduncan
Copy link
Contributor Author

It does sound like we could probably just release another version of each of our "one-version-only" artifacts and encourage people to do that, but maybe we'll be able to do something larger, too.

Sorry @cpovirk, I am struggling to understand what this sentence means. Would you mind clarifying things for me? :)

@cpovirk
Copy link
Member

cpovirk commented Apr 15, 2019

I... uh... I'm not sure. Sorry.

I'm guessing that "do" was supposed to be "use?" The idea is: We'd release a new failureaccess and a new listenablefuture, each without the parent, and then recommend that people use that.

@jbduncan
Copy link
Contributor Author

Thanks for the clarification, @cpovirk!

I don't suppose you've had the time to continue looking into this issue (which I acknowledge is very hairy), have you? :)

@raghsriniv raghsriniv added the P3 label Jun 24, 2019
@electrum
Copy link

We are another holdout who plan to stick with 26.0-jre for the foreseeable future.

Have there been any further discussions on @jodastephen's proposal, which is now at 50 votes, to revert the -jre version to have no dependencies? From what I can tell, this change was made (primarily) for Android, and not a single non-Android user is supportive of it.

@talios
Copy link

talios commented Jul 23, 2019

I made the switch - and because we exclude all transitives found that (in our codebase) failureaccess was only needed to be added explicitly to about 3-4 modules (out of 80ish) due to usage of Guava Caches, which fell into internal usage of futures.

Thankfully, I could get away with adding it as a test dependency, along with inclusion in the final distribution - but it feels... wrong.

@jbduncan
Copy link
Contributor Author

jbduncan commented Dec 5, 2019

Hey @cpovirk - I don't suppose you've had any time recently to look into this issue some more, have you?

ben-manes added a commit to ben-manes/caffeine that referenced this issue Dec 13, 2019
OSGi loads bundles as atomic units on a classpath, so split packages
are not supported. When the class verifier is enabled, it fails to
find required class dependencies and the test fails. When disabled
the test passes since we do not need to load the missing class.

Disabling verifying is a deprecated feature in JDK13 for removal. It
was originally added to speed up tests, since resolved by reflectively
loading classes in the factories rather than explicit class references.

google/guava#3320
@jodastephen
Copy link
Contributor

As it stands, I have no desire to upgrade Guava beyond v26, which could be a problem if there is a security issue.

So, I now have this problem. The security world is screaming that Guava has a security hole (even if it doesn't really) but I have no desire to upgrade while Guava continues to have this dependency mess.

Can we get a resolution to this issue please?

@cpovirk
Copy link
Member

cpovirk commented Nov 2, 2020

Sorry for the continued trouble. If I were doing this again today, I can't see myself introducing the dependency. At this point, though, we can't remove it without making a binary-incompatible change to AbstractFuture (and yes, that's another reason that we should have erred on the side of leaving it out :\) or without introducing further build weirdness that is likely to cause harm on net.

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

No branches or pull requests