Tembrel commented on this pull request.
> + * @return a new instance of SimpleTimeLimiter that uses executorService
+ */
+ public static SimpleTimeLimiter createSimpleTimeLimiter(ExecutorService
executorService) {
+ try {
+ if (CREATE_STL != null) {
+ return (SimpleTimeLimiter) CREATE_STL.invoke(null,
executorService);
+ } else if (CONSTRUCT_STL != null) {
+ return CONSTRUCT_STL.newInstance(executorService);
+ }
+ // fall through
+ } catch (IllegalAccessException iae) {
+ // fall through
+ } catch (InstantiationException ie) {
+ // fall through
+ } catch (InvocationTargetException ite) {
+ throw Throwables.propagate(ite.getCause());
Earlier you wrote "It would be better to propagate the cause of these
exceptions", but the only one of the exceptions in the three catch clauses that
has a (relevant) cause is `InvocationTargetException`. Maybe you meant
"propagate these exceptions to indicate the cause of the failure"?
Freed from that misunderstanding, here's my analysis. There are four failure
cases:
1. Neither `SimpleTypeLimiter.create(ExecutorService)` nor
`SimpleTypeLimiter(ExecutorService)` was found. Should never happen. but if it
did, the `NoSuchMethodException` that gave rise to this state has been lost by
this point.
1. `IllegalAccessException`: don't have access to the method or constructor.
1. `InstantiationException`: constructor invocation failed.
1. `InvocationTargetException`: the method or constructor invocation threw an
exception.
In the first case, there's no exception to propagate, so
`UnsupportedMethodException` is a pretty good description of the situation. In
the second and third cases, these checked exceptions could be wrapped in an
unchecked exception and rethrown.
In the last case, the cause of the `InvocationTargetException` is the main
point. The `createSimpleTimeLimiter` method is a substitute for either a call
to `SimpleTimeLimiter.create(exec)` or `new SimpleTimeLimiter(exec)`, so we'd
like its semantics to mirror those of the things it's replacing as closely as
possible. I think it would better to unwrap the cause and then wrap in an
unchecked exception if necessary before rethrowing.
I stopped using `Throwables.propagate` when Guava deprecated it, but JClouds'
`TypeTokenUtils`, on which I modeled this PR, **does** use
`Throwables.propagate`. But I'm happy to wrap with an unchecked exception
instead when necessary.
However, I'm not convinced that `RuntimeException` is really the best choice
for wrapping checked exceptions. For one thing, my linting tools frown on using
such a generic exception type if there's a more subtype that's more
specifically appropriate. In this case, I think `UnsupportedOperationException`
is a better choice for all of the failure cases except when the wrapped
invocation target exception is already unchecked.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1133#discussion_r135400053
