Skip to content

Commit

Permalink
Remove ClassValue implementation of Futures.getChecked from the A…
Browse files Browse the repository at this point in the history
…ndroid flavor.

It doesn't work there, even under new versions of Android, so it always triggers fallback, and the process of falling back burns resources and produces noise.

(JRE users of guava-android could benefit from the `ClassValue` implementation, but now that we're dropping support for Java 7 from guava-android, there's no reason for them to use it. OK, probably some users run _Robolectric_ tests against guava-android. But presumably they aren't _too_ sensitive to the performance of `getChecked` in the failure case.)

Removing `ClassValue` support also lets us remove a bunch of Animal-Sniffer cruft. That includes upgrading it to 1.19, which in turn forces us to remove some usages of new APIs from our _tests_ -- not nearly as important as usages in _prod_ but still a good idea so that any future Android test failures are reported to us correctly instead of hidden by `NoSuchMethodError` or similar. Or actually, Animal-Sniffer 1.20 [turned checking of test sources back off](mojohaus/animal-sniffer@ac40ac4), but let's opt in anyway, at least to see how it goes, at least for the tests of guava-testlib. (guava-tests itself is more of a pain, so I left a TODO.)

RELNOTES=n/a
PiperOrigin-RevId: 391779058
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Aug 19, 2021
1 parent c7d9fef commit fb109b0
Show file tree
Hide file tree
Showing 14 changed files with 36 additions and 135 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,8 @@ public Object invoke(Object target, Method method, Object[] args) throws Throwab
return method.invoke(map, args);
} catch (InvocationTargetException e) {
throw e.getCause();
} catch (ReflectiveOperationException e) {
throw new LinkageError(e.getMessage(), e);
} catch (IllegalAccessException e) {
throw newLinkageError(e);
}
}
}
Expand Down Expand Up @@ -338,4 +338,10 @@ public void run() {
.withTearDown(tearDown)
.createTestSuite();
}

private static LinkageError newLinkageError(Throwable cause) {
LinkageError error = new LinkageError(cause.toString());
error.initCause(cause);
return error;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static com.google.common.util.concurrent.FuturesGetChecked.checkExceptionClassValidity;
import static com.google.common.util.concurrent.FuturesGetChecked.classValueValidator;
import static com.google.common.util.concurrent.FuturesGetChecked.getChecked;
import static com.google.common.util.concurrent.FuturesGetChecked.isCheckedException;
import static com.google.common.util.concurrent.FuturesGetChecked.weakSetValidator;
Expand Down Expand Up @@ -52,7 +51,7 @@ private enum Validator {
NON_CACHING_WITH_CONSTRUCTOR_CHECK(nonCachingWithConstructorCheckValidator()),
NON_CACHING_WITHOUT_CONSTRUCTOR_CHECK(nonCachingWithoutConstructorCheckValidator()),
WEAK_SET(weakSetValidator()),
CLASS_VALUE(classValueValidator());
;

final GetCheckedTypeValidator validator;

Expand Down
3 changes: 3 additions & 0 deletions android/guava-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>animal-sniffer-maven-plugin</artifactId>
<configuration>
<checkTestClasses>false</checkTestClasses> <!-- TODO(cpovirk): Consider checking them. -->
</configuration>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,5 +381,8 @@ private WeakReference<?> doTestClassUnloading() throws Exception {
* environment that forces Futures.getChecked to its fallback WeakSetValidator. One awful way of
* doing so would be to derive a separate test library by using remove_from_jar to strip out
* ClassValueValidator.
*
* Fortunately, we get pretty good coverage "by accident": We run all these tests against the
* *backport*, where ClassValueValidator is not present.
*/
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import com.google.common.base.Function;
import com.google.common.collect.Ordering;
import com.google.errorprone.annotations.CanIgnoreReturnValue;
import com.google.j2objc.annotations.J2ObjCIncompatible;
import java.lang.ref.WeakReference;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
Expand Down Expand Up @@ -101,12 +100,6 @@ static GetCheckedTypeValidator weakSetValidator() {
return GetCheckedTypeValidatorHolder.WeakSetValidator.INSTANCE;
}

@J2ObjCIncompatible // ClassValue
@VisibleForTesting
static GetCheckedTypeValidator classValueValidator() {
return GetCheckedTypeValidatorHolder.ClassValueValidator.INSTANCE;
}

/**
* Provides a check of whether an exception type is valid for use with {@link
* FuturesGetChecked#getChecked(Future, Class)}, possibly using caching.
Expand All @@ -115,35 +108,8 @@ static GetCheckedTypeValidator classValueValidator() {
*/
@VisibleForTesting
static class GetCheckedTypeValidatorHolder {
static final String CLASS_VALUE_VALIDATOR_NAME =
GetCheckedTypeValidatorHolder.class.getName() + "$ClassValueValidator";

static final GetCheckedTypeValidator BEST_VALIDATOR = getBestValidator();

@IgnoreJRERequirement // getChecked falls back to another implementation if necessary
@J2ObjCIncompatible // ClassValue
enum ClassValueValidator implements GetCheckedTypeValidator {
INSTANCE;

/*
* Static final fields are presumed to be fastest, based on our experience with
* UnsignedBytesBenchmark. TODO(cpovirk): benchmark this
*/
private static final ClassValue<Boolean> isValidClass =
new ClassValue<Boolean>() {
@Override
protected Boolean computeValue(Class<?> type) {
checkExceptionClassValidity(type.asSubclass(Exception.class));
return true;
}
};

@Override
public void validateClass(Class<? extends Exception> exceptionClass) {
isValidClass.get(exceptionClass); // throws if invalid; returns safely (and caches) if valid
}
}

enum WeakSetValidator implements GetCheckedTypeValidator {
INSTANCE;

Expand Down Expand Up @@ -190,13 +156,7 @@ public void validateClass(Class<? extends Exception> exceptionClass) {
* unable to do so.
*/
static GetCheckedTypeValidator getBestValidator() {
try {
Class<? extends Enum> theClass =
Class.forName(CLASS_VALUE_VALIDATOR_NAME).asSubclass(Enum.class);
return (GetCheckedTypeValidator) theClass.getEnumConstants()[0];
} catch (Throwable t) { // ensure we really catch *everything*
return weakSetValidator();
}
return weakSetValidator();
}
}

Expand Down

This file was deleted.

20 changes: 2 additions & 18 deletions android/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,7 @@
<test.include>%regex[.*.class]</test.include>
<truth.version>1.1.2</truth.version>
<checker-framework.version>3.12.0</checker-framework.version>
<!--
Upgrading to 1.19 breaks things: Animal Sniffer reports a problem with the
use of ClassValue in FuturesGetChecked, even though we've added a
suppression annotation.
The problem might be
https://github.com/mojohaus/animal-sniffer/issues/131.
We could probably work around this with a different suppression strategy,
perhaps (untested):
<ignores><ignore>com.google.common.util.concurrent.FuturesGetChecked$GetCheckedTypeValidatorHolder$ClassValueValidator</ignore></ignores>
But upgrading doesn't really buy us much, as far as I know. (1.19 does
check test code in addition to prod code, but I don't think we care.)
-->
<animal.sniffer.version>1.18</animal.sniffer.version>
<animal.sniffer.version>1.20</animal.sniffer.version>
<maven-javadoc-plugin.version>3.1.0</maven-javadoc-plugin.version>
<maven-source-plugin.version>3.2.1</maven-source-plugin.version>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
Expand Down Expand Up @@ -167,7 +151,7 @@
<artifactId>animal-sniffer-maven-plugin</artifactId>
<version>${animal.sniffer.version}</version>
<configuration>
<annotations>com.google.common.util.concurrent.IgnoreJRERequirement</annotations>
<checkTestClasses>true</checkTestClasses>
<signature>
<groupId>org.codehaus.mojo.signature</groupId>
<artifactId>java16-sun</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -313,8 +313,8 @@ public Object invoke(Object target, Method method, Object[] args) throws Throwab
return method.invoke(map, args);
} catch (InvocationTargetException e) {
throw e.getCause();
} catch (ReflectiveOperationException e) {
throw new LinkageError(e.getMessage(), e);
} catch (IllegalAccessException e) {
throw newLinkageError(e);
}
}
}
Expand Down Expand Up @@ -349,4 +349,10 @@ public void run() {
.withTearDown(tearDown)
.createTestSuite();
}

private static LinkageError newLinkageError(Throwable cause) {
LinkageError error = new LinkageError(cause.toString());
error.initCause(cause);
return error;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,8 @@ private enum Validator {
NON_CACHING_WITH_CONSTRUCTOR_CHECK(nonCachingWithConstructorCheckValidator()),
NON_CACHING_WITHOUT_CONSTRUCTOR_CHECK(nonCachingWithoutConstructorCheckValidator()),
WEAK_SET(weakSetValidator()),
CLASS_VALUE(classValueValidator());
CLASS_VALUE(classValueValidator()),
;

final GetCheckedTypeValidator validator;

Expand Down
3 changes: 3 additions & 0 deletions guava-tests/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>animal-sniffer-maven-plugin</artifactId>
<configuration>
<checkTestClasses>false</checkTestClasses> <!-- TODO(cpovirk): Consider checking them. -->
</configuration>
</plugin>
<plugin>
<groupId>org.codehaus.mojo</groupId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -381,5 +381,8 @@ private WeakReference<?> doTestClassUnloading() throws Exception {
* environment that forces Futures.getChecked to its fallback WeakSetValidator. One awful way of
* doing so would be to derive a separate test library by using remove_from_jar to strip out
* ClassValueValidator.
*
* Fortunately, we get pretty good coverage "by accident": We run all these tests against the
* *backport*, where ClassValueValidator is not present.
*/
}
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ static class GetCheckedTypeValidatorHolder {

static final GetCheckedTypeValidator BEST_VALIDATOR = getBestValidator();

@IgnoreJRERequirement // getChecked falls back to another implementation if necessary
@J2ObjCIncompatible // ClassValue
enum ClassValueValidator implements GetCheckedTypeValidator {
INSTANCE;
Expand Down

This file was deleted.

20 changes: 2 additions & 18 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,23 +16,7 @@
<test.include>%regex[.*.class]</test.include>
<truth.version>1.1.2</truth.version>
<checker-framework.version>3.12.0</checker-framework.version>
<!--
Upgrading to 1.19 breaks things: Animal Sniffer reports a problem with the
use of ClassValue in FuturesGetChecked, even though we've added a
suppression annotation.
The problem might be
https://github.com/mojohaus/animal-sniffer/issues/131.
We could probably work around this with a different suppression strategy,
perhaps (untested):
<ignores><ignore>com.google.common.util.concurrent.FuturesGetChecked$GetCheckedTypeValidatorHolder$ClassValueValidator</ignore></ignores>
But upgrading doesn't really buy us much, as far as I know. (1.19 does
check test code in addition to prod code, but I don't think we care.)
-->
<animal.sniffer.version>1.18</animal.sniffer.version>
<animal.sniffer.version>1.20</animal.sniffer.version>
<maven-javadoc-plugin.version>3.1.0</maven-javadoc-plugin.version>
<maven-source-plugin.version>3.2.1</maven-source-plugin.version>
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
Expand Down Expand Up @@ -168,7 +152,7 @@
<artifactId>animal-sniffer-maven-plugin</artifactId>
<version>${animal.sniffer.version}</version>
<configuration>
<annotations>com.google.common.util.concurrent.IgnoreJRERequirement</annotations>
<checkTestClasses>true</checkTestClasses>
<signature>
<groupId>org.codehaus.mojo.signature</groupId>
<artifactId>java18</artifactId>
Expand Down

0 comments on commit fb109b0

Please sign in to comment.