-
Notifications
You must be signed in to change notification settings - Fork 10.9k
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
JDK and Guava TypeVariable implementations are no longer compatible under 1.7.0_51-b13 #1635
Comments
Original comment posted by wasserman.louis on 2014-01-17 at 03:02 AM Can you provide stack traces so we might have a better idea of what is going wrong in your code? Our tests pass internally. Labels: |
Original comment posted by magiciankid on 2014-01-17 at 08:29 AM The testcase already exists in the project , not my own . Here is the stack traces :T E S T SRunning com.google.common.reflect.AbstractInvocationHandlerTest Jan 17, 2014 4:26:46 PM com.google.common.reflect.ClassPath$Scanner getClassPathFromManifest Results : Failed tests: Tests run: 1652, Failures: 14, Errors: 0, Skipped: 0 |
Original comment posted by magiciankid on 2014-01-17 at 08:33 AM Here is the command : |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:08 PM Things are OK at 1.7.0_45 but definitely broken at 1.7.0_51-b13 I wonder if the problem is in the JDK itself. Maybe they broke their equals() methods? Here are some stack traces: Running com.google.common.reflect.TypeResolverTest testWhere_typeVariableSelfMapping(com.google.common.reflect.TypeResolverTest) Time elapsed: 0.001 sec <<< FAILURE! testWhere_parameterizedSelfMapping(com.google.common.reflect.TypeResolverTest) Time elapsed: 0.002 sec <<< FAILURE! testWhere_genericArraySelfMapping(com.google.common.reflect.TypeResolverTest) Time elapsed: 0 sec <<< FAILURE! testWhere_wildcardSelfMapping(com.google.common.reflect.TypeResolverTest) Time elapsed: 0.002 sec <<< FAILURE! Tests run: 26, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.066 sec <<< FAILURE! Running com.google.common.reflect.TypeTokenResolutionTest testConextIsParameterizedType(com.google.common.reflect.TypeTokenResolutionTest) Time elapsed: 0 sec <<< FAILURE! testGenericArrayType(com.google.common.reflect.TypeTokenResolutionTest) Time elapsed: 0 sec <<< FAILURE! Running com.google.common.reflect.TypeTokenTest testGetSupertype_fromRawClass(com.google.common.reflect.TypeTokenTest) Time elapsed: 0 sec <<< FAILURE! testGetSupertype_fullyGenericType(com.google.common.reflect.TypeTokenTest) Time elapsed: 0 sec <<< FAILURE! testGetSupertype_partiallySpecializedType(com.google.common.reflect.TypeTokenTest) Time elapsed: 0 sec <<< FAILURE! testWhere(com.google.common.reflect.TypeTokenTest) Time elapsed: 0.001 sec <<< FAILURE! CC: benyu@google.com |
Original comment posted by cgdecker@google.com on 2014-01-17 at 06:12 PM Yeah, I was just testing it and got the same thing. It does seem likely that the problem is in the JDK. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:20 PM I looked a little more at TypeTokenResolutionTest#testConextIsParameterizedType, which seemed like one of the simpler tests. Under both JDK versions, keyType is a sun.reflect.generics.reflectiveObjects.TypeVariableImpl, and TypeToken.of(keyType).resolveType(keyType).getType() is a com.google.common.reflect.Types$TypeVariableImpl. The tests pass if I reverse the order of the arguments to assertEquals. This further supports the theory that the JDK equals() method is broken. More explicitly, this test passes, but the reverse does not: assertTrue(TypeToken.of(keyType).resolveType(keyType).getType().equals(keyType)); However, I also diffed the output of javap -v for sun.reflect.generics.reflectiveObjects.TypeVariableImpl between the JDKs, and it matches exactly. Of course, the problem may lie in a method called by TypeVariableImpl. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:23 PM Err, no, I think I got that last part wrong. I'm running javap -cp jre/lib/rt.jar, but I'm probably getting the version from the bootstrap classpath. I'll redo it. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:29 PM Sure enough, the new TypeVariableImpl is different. It does an instanceof check and a getClass() == TypeVariableImpl.class check. Odd. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:36 PM I tried to find the location of the JDK's TypeVariableImpl. However, the copy I see in Mercurial looks like the 1.7.0_45 version that works fine with our tests. Maybe I need to find a branch or an Oracle-specific repository. I'm not familiar with the JDK release process. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:38 PM In fact, that whole repository seems to have not been touched in >2 years. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 06:43 PM Well, I see the change in a JDK8 repository: The change is listed under "Security fixes" in some release notes: "S8023301: Enhance generic classes" http://mail.openjdk.java.net/pipermail/distro-pkg-dev/2014-January/025800.html |
Original comment posted by cgdecker@google.com on 2014-01-17 at 06:59 PM In hg, you need to look in jdk7u/jdk7u, not jdk7/jdk7. jdk7u is where update work goes. |
Original comment posted by cgdecker@google.com on 2014-01-17 at 07:02 PM Here's the change that introduced the "o.getClass() == TypeVariableImpl.class" check: http://hg.openjdk.java.net/jdk7u/jdk7u/jdk/diff/8e542d28b8ec/src/share/classes/sun/reflect/generics/reflectiveObjects/TypeVariableImpl.java |
Original comment posted by cgdecker@google.com on 2014-01-17 at 07:13 PM Issue #1637 has been merged into this issue. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 07:14 PM Issue #1637 has been merged into this issue. |
Original comment posted by cpovirk@google.com on 2014-01-17 at 07:16 PM (No comment entered for this change.) |
Original comment posted by john....@socrata.com on 2014-01-20 at 04:55 PM I appears that this was closed resolved in their JIRA tracker: |
Original comment posted by frankoid on 2014-01-20 at 05:47 PM To my eyes the description in https://bugs.openjdk.java.net/browse/JDK-8031984 doesn't make a very compelling case that this is a bug in the JDK - would it be possible to make a more compelling case there, for example by having a test case that doesn't use internal sun.reflect classes? |
Original comment posted by sharedocs1 on 2014-01-20 at 06:07 PM
Trying to take a quick step back here: is this actually a bug? I mean, is there any kind of documented contract that says that 'equals' on TypeVariables should allow for other implementing classes? The Javadoc for 7 [1] does say: "However, all instances representing a type variable must be equal() to each other. As a consequence, users of type variables must not rely on the identity of instances of classes implementing this interface." Could one use this as justification for calling this a bug? I guess Oracle might argue that users shouldn't be creating "instances representing a type variable" on their own in the first place? [1] http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/TypeVariable.html |
Original comment posted by nwilton on 2014-01-20 at 08:16 PM This is a bug. I now have a production failure which has been associated to this, created due to automatic CentOS updates. I recommend that a fix is created and released ASAP, and we worry about the semantics later. |
Original comment posted by sharedocs1 on 2014-01-20 at 08:21 PM
Fully agree with that (this has broken a lot of stuff on our side, too). I should have made it clearer that I wasn't talking about this ticket, but about Oracle's response in closing the issue raised against the JDK. |
Original comment posted by benyu@google.com on 2014-01-21 at 12:46 AM Some of these broken tests could be worked around by checking whether the component types have changed through transformation, or else use the original TypeVariable instance. if (bounds == originalBounds) { This doesn't address the case where the type variable bound went through transformation. For that, options I can think of:
Neither feels good enough. |
Original comment posted by diwakergupta on 2014-01-21 at 06:50 AM Note that this breaks downstream libraries that depend on guava as well, so this isn't just an issue of test code in Guava. Here's just one example: https://issues.apache.org/jira/browse/JCLOUDS-427 Given the number of proprietary and open source projects that depend on Guava, and the fact that many systems will have silent, automatic upgrades of the JDK, I suspect this will impact a large number of users soon if it hasn't already. In fact, due to dependency conflicts, we are not able to upgrade to Guava 16 yet so a backport to Guava 15.x will be hugely appreciated! |
Original comment posted by cpovirk@google.com on 2014-01-23 at 11:54 PM Suppose that we want to create an instance of the JDK's TypeVariableImpl for List<String>. Might we be able to use something like ASM/cglib to generate a custom type, HasTypeParameterForListString<T extends List<String>>? Then we could ask standard Java reflection for HasTypeParameterForListString's type parameter's bound, at which point we'd get a JDK TypeVariableImpl back. |
Original comment posted by tavianator on 2014-01-24 at 12:02 AM But that TypeVariableImpl wouldn't have the right GenericDeclaration, so it still wouldn't compare equal. |
Original comment posted by cpovirk@google.com on 2014-01-24 at 12:20 AM Oh, I see. My attempted solution doesn't account for retrieving type variables at all. The TypeVariableImpl that I'm retrieving is for <T extends List<String>>. But I don't care about that; I care about List<String>. I can get it from getBounds(), and that part works fine (I suspect). But what if I want List<E>, where E is declared in some random class or method? (After all, it's the TypeVariable for E that's the problem here.) I doubt there's a general way to declare a type that refers to such a thing. But I'd love for someone to prove me wrong.... |
Original comment posted by cpovirk@google.com on 2014-01-24 at 04:51 AM We may try the terrible thing -- manually creating sun.reflect.generics types. But we would do it only in the presence of the new, problematic TypeVariableImpl and only under the guard of reflection. I'm running some tests internally now to see how well an early attempt works. |
Original comment posted by nick.wil...@jds.net.au on 2014-01-24 at 11:15 AM This defect is receiving a great deal of attention from the wider community: I suggest that a fix is identified as soon as possible to resolve this defect and minimise further impact on Guava users. I've also opened Issue #1644 to work on removing all dependency on unsupported API classes. |
Original comment posted by kevinb@google.com on 2014-01-24 at 04:32 PM Chris, "under the guard of reflection" seems to imply some sort of fallback, but to what? |
Original comment posted by cpovirk@google.com on 2014-01-24 at 05:14 PM To the current behavior. Jeremy points out that, as far as we know[*], there are two kinds of reflection-enabled Java environments in the world:
[*] Yes, that's not very encouraging. |
Original comment posted by tavianator on 2014-01-24 at 05:37 PM RE: #22, if the bounds go through transformation, does it even make sense to have the new TypeVariable compare equal to the old one? |
Original comment posted by benyu@google.com on 2014-01-24 at 05:50 PM Yeah. That's a good question. I'm having a hard time imagining why it would matter. So I think it might be a good solution. Need to give it a bit more thought. And probably start experimenting it. Thanks! |
Original comment posted by jgl...@cloudbees.com on 2014-01-24 at 09:46 PM
There seems to be no explicit prohibition; the closest I can find is this note earlier in the Javadoc:
which vaguely implies that implementations outside the JRE are not supported. Since Java provides no way of restricting what code may implement an interface (other than using annotations and annotation processors not defined in the Platform), API documentation should clearly state how an interface may be used. In this case the designers of the reflection API apparently failed to consider the possibility. |
Original comment posted by sharedocs1 on 2014-01-27 at 07:18 PM For what it's worth, we (jclouds) have been able to workaround the failures this was causing by switching to other Guava reflection methods. See https://issues.apache.org/jira/browse/JCLOUDS-427 for details. Obviously, this is not a solution to the ticket, but might help other users that call similar Guava functions. |
Original comment posted by ckutzi on 2014-01-29 at 02:46 PM Does someone know of a simple test to check if an application would be affected by this bug - also considering the possibility that the app is not using the 'broken' methods directly but only indirectly via a 'downstream' library? |
Original comment posted by sharedocs1 on 2014-01-29 at 07:54 PM
Would this be a test you would want to include in Guava test suite, or something a Guava user (directly or via downstream libs) could run against his/her application to test this? I presume that, in the latter case, the easiest thing would probably be to run the app against 7u51 to see what happens..? |
Original comment posted by ckutzi on 2014-01-29 at 09:43 PM The latter one; Something a Guava user (directly or via downstream libs) could run against his/her application. It's a bigger 'enterprise' system consisting of many services/apps of whom many directly or indirectly depend on Guava. |
Original comment posted by sharedocs1 on 2014-01-30 at 12:20 AM
Do you have some kind of static checker in mind here that could see if you could run into this problem in one of your code paths? Something like a "vulnerability scanner" that a user might run before deciding whether or not to move to 7u51? I'm guessing the challenge here is that one would obviously want to flag only invocations of the affected methods (directly in app code or via a downstream lib) that will actually be called when the app is running in real life? I.e. rather than simply flagging up every invocation of an affected method anywhere in the app or downstream libs, even if they will never be invoked. |
Original comment posted by jgl...@cloudbees.com on 2014-01-30 at 02:02 PM Seems like the time spent implementing such a static checker would greatly exceed the time needed to just work around the JRE behavior in Guava and release a Guava update. (Ideally with emergency patch updates to 11.x and subsequent major releases, to allow people stuck for compatibility reasons with older Guava versions to keep running.) Of course there is probably already some tool out there which traverses the static call tree of an app (starting from bytecode you wrote) looking for references to a given class or method. |
Original comment posted by cpovirk@google.com on 2014-02-03 at 09:07 PM The fix has been submitted: We'll put out a patch release for Guava 16 that includes this fix (and a forthcoming revision to its documentation). Status: |
Original comment posted by nwilton on 2014-02-03 at 09:12 PM Fantastic! Can you also please backport this fix to Guava 15? |
Original comment posted by sharedocs1 on 2014-02-03 at 09:14 PM
Wow, great! Thanks! |
Original comment posted by cgdecker@google.com on 2014-02-03 at 09:15 PM @nwilton: Are you unable to switch to Guava 16 for some reason? Just wondering. |
Original comment posted by cgdecker@google.com on 2014-02-04 at 06:55 PM 16.0.1 has been released with this fix: http://search.maven.org/#artifactdetails%7Ccom.google.guava%7Cguava%7C16.0.1%7Cbundle Status: |
Original comment posted by g...@maginatics.com on 2014-02-05 at 09:04 PM Any plans for 15.0.1 with the same fix? jclouds upgraded to 16.0.1 in its 1.8.x development branch but prefers not to upgrade Guava a major version in its 1.7.x stable branch due to various Guava deprecations and API changes. |
Original issue created by magiciankid on 2014-01-17 at 01:02 AM
com.google.common.reflect.TypeResolverTest
com.google.common.reflect.TypeTokenResolutionTest
com.google.common.reflect.TypeTokenTest
com.google.common.reflect.TypesTest
java -version
java version "1.7.0_51"
Java(TM) SE Runtime Environment (build 1.7.0_51-b13)
Java HotSpot(TM) 64-Bit Server VM (build 24.51-b03, mixed mode)
please fix it :)
The text was updated successfully, but these errors were encountered: