jclouds-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Tim Peierls <notificati...@github.com>
Subject Re: [jclouds/jclouds] JCLOUDS-1334: Guava 23 compatibility (SimpleTimeLimiter creation) (#1133)
Date Sun, 27 Aug 2017 00:02:14 GMT
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
Mime
View raw message