Skip to content

Commit

Permalink
Restore the behavior of throwing an IllegalArgumentException directly.
Browse files Browse the repository at this point in the history
I'd undone this in cl/686728158 as part of addressing #7434, but I subsequently got a guilty conscience. It almost certainly doesn't matter, but it's a little weird.

RELNOTES=n/a
PiperOrigin-RevId: 686919814
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Oct 17, 2024
1 parent dc423d2 commit 585b93a
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 14 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@

package com.google.common.eventbus;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.UncheckedExecutionException;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
Expand Down Expand Up @@ -283,10 +281,7 @@ class SubscribesToPrimitive {
@Subscribe
public void toInt(int i) {}
}
UncheckedExecutionException thrown =
assertThrows(
UncheckedExecutionException.class, () -> bus.register(new SubscribesToPrimitive()));
assertThat(thrown).hasCauseThat().isInstanceOf(IllegalArgumentException.class);
assertThrows(IllegalArgumentException.class, () -> bus.register(new SubscribesToPrimitive()));
}

/** Records thrown exception information. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.common.collect.Multimap;
import com.google.common.primitives.Primitives;
import com.google.common.reflect.TypeToken;
import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.j2objc.annotations.Weak;
import java.lang.reflect.Method;
import java.util.Arrays;
Expand Down Expand Up @@ -164,7 +165,29 @@ private Multimap<Class<?>, Subscriber> findAllSubscribers(Object listener) {
}

private static ImmutableList<Method> getAnnotatedMethods(Class<?> clazz) {
return subscriberMethodsCache.getUnchecked(clazz);
try {
return subscriberMethodsCache.getUnchecked(clazz);
} catch (UncheckedExecutionException e) {
if (e.getCause() instanceof IllegalArgumentException) {
/*
* IllegalArgumentException is the one unchecked exception that we know is likely to happen
* (thanks to the checkArgument calls in getAnnotatedMethodsNotCached). If it happens, we'd
* prefer to propagate an IllegalArgumentException to the caller. However, we don't want to
* simply rethrow an exception (e.getCause()) that may in rare cases have come from another
* thread. To accomplish both goals, we wrap that IllegalArgumentException in a new
* instance.
*/
throw new IllegalArgumentException(e.getCause().getMessage(), e.getCause());
}
/*
* If some other exception happened, we just propagate the wrapper
* UncheckedExecutionException, which has the stack trace from this thread and which has its
* cause set to the underlying exception (which may be from another thread). If we someday
* learn that some other exception besides IllegalArgumentException is common, then we could
* add another special case to throw an instance of it, too.
*/
throw e;
}
}

private static ImmutableList<Method> getAnnotatedMethodsNotCached(Class<?> clazz) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,10 @@

package com.google.common.eventbus;

import static com.google.common.truth.Truth.assertThat;
import static org.junit.Assert.assertThrows;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.Lists;
import com.google.common.util.concurrent.UncheckedExecutionException;
import java.util.List;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
Expand Down Expand Up @@ -283,10 +281,7 @@ class SubscribesToPrimitive {
@Subscribe
public void toInt(int i) {}
}
UncheckedExecutionException thrown =
assertThrows(
UncheckedExecutionException.class, () -> bus.register(new SubscribesToPrimitive()));
assertThat(thrown).hasCauseThat().isInstanceOf(IllegalArgumentException.class);
assertThrows(IllegalArgumentException.class, () -> bus.register(new SubscribesToPrimitive()));
}

/** Records thrown exception information. */
Expand Down
25 changes: 24 additions & 1 deletion guava/src/com/google/common/eventbus/SubscriberRegistry.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import com.google.common.collect.Multimap;
import com.google.common.primitives.Primitives;
import com.google.common.reflect.TypeToken;
import com.google.common.util.concurrent.UncheckedExecutionException;
import com.google.j2objc.annotations.Weak;
import java.lang.reflect.Method;
import java.util.Arrays;
Expand Down Expand Up @@ -164,7 +165,29 @@ private Multimap<Class<?>, Subscriber> findAllSubscribers(Object listener) {
}

private static ImmutableList<Method> getAnnotatedMethods(Class<?> clazz) {
return subscriberMethodsCache.getUnchecked(clazz);
try {
return subscriberMethodsCache.getUnchecked(clazz);
} catch (UncheckedExecutionException e) {
if (e.getCause() instanceof IllegalArgumentException) {
/*
* IllegalArgumentException is the one unchecked exception that we know is likely to happen
* (thanks to the checkArgument calls in getAnnotatedMethodsNotCached). If it happens, we'd
* prefer to propagate an IllegalArgumentException to the caller. However, we don't want to
* simply rethrow an exception (e.getCause()) that may in rare cases have come from another
* thread. To accomplish both goals, we wrap that IllegalArgumentException in a new
* instance.
*/
throw new IllegalArgumentException(e.getCause().getMessage(), e.getCause());
}
/*
* If some other exception happened, we just propagate the wrapper
* UncheckedExecutionException, which has the stack trace from this thread and which has its
* cause set to the underlying exception (which may be from another thread). If we someday
* learn that some other exception besides IllegalArgumentException is common, then we could
* add another special case to throw an instance of it, too.
*/
throw e;
}
}

private static ImmutableList<Method> getAnnotatedMethodsNotCached(Class<?> clazz) {
Expand Down

0 comments on commit 585b93a

Please sign in to comment.