Allow users to override timeouts on sync interface: some refactoring, unit test added. See issue #253

core/src/main/java/org/jclouds/concurrent/internal/SyncProxy.java

@@ -19,7 +19,6 @@
 package org.jclouds.concurrent.internal;
 
 import static com.google.common.base.Preconditions.checkState;
-import static org.jclouds.Constants.PROPERTY_TIMEOUTS_PREFIX;
 
 import java.lang.reflect.InvocationHandler;
 import java.lang.reflect.Method;
@@ -55,9 +54,12 @@ import com.google.inject.ProvisionException;
 public class SyncProxy implements InvocationHandler {
 
    @SuppressWarnings("unchecked")
-   public static <T> T proxy(Class<T> clazz, SyncProxy proxy) throws IllegalArgumentException, SecurityException,
+   public static <T> T proxy(Class<T> clazz, Object async,
+         @Named("sync") Cache<ClassMethodArgs, Object> delegateMap,
+         Map<Class<?>, Class<?>> sync2Async, Map<String, Long> timeouts) throws IllegalArgumentException, SecurityException,
          NoSuchMethodException {
-      return (T) Proxy.newProxyInstance(clazz.getClassLoader(), new Class<?>[] { clazz }, proxy);
+      return (T) Proxy.newProxyInstance(clazz.getClassLoader(), new Class<?>[] { clazz },
+              new SyncProxy(clazz, async, delegateMap, sync2Async, timeouts));
    }
 
    private final Object delegate;
@@ -70,15 +72,9 @@ public class SyncProxy implements InvocationHandler {
    private static final Set<Method> objectMethods = ImmutableSet.of(Object.class.getMethods());
 
    @Inject
-   public void setProperties(@Named("CONSTANTS") Multimap<String, String> props) {
+   private SyncProxy(Class<?> declaring, Object async,
-       for (final Method method : timeoutMap.keySet()) {
+         @Named("sync") Cache<ClassMethodArgs, Object> delegateMap, Map<Class<?>,
-             overrideTimeout(declaring, method, props);
+           Class<?>> sync2Async, final Map<String, Long> timeouts)
-       }
-   }
-
-   @Inject
-   public SyncProxy(Class<?> declaring, Object async,
-         @Named("sync") Cache<ClassMethodArgs, Object> delegateMap, Map<Class<?>, Class<?>> sync2Async)
          throws SecurityException, NoSuchMethodException {
       this.delegateMap = delegateMap;
       this.delegate = async;
@@ -100,14 +96,7 @@ public class SyncProxy implements InvocationHandler {
                throw new IllegalArgumentException(String.format(
                      "method %s has different typed exceptions than delegated method %s", method, delegatedMethod));
             if (delegatedMethod.getReturnType().isAssignableFrom(ListenableFuture.class)) {
-               if (method.isAnnotationPresent(Timeout.class)) {
+               timeoutMap.put(method, getTimeout(method, typeNanos, timeouts));
-                  Timeout methodTimeout = method.getAnnotation(Timeout.class);
-                  long methodNanos = convertToNanos(methodTimeout);
-                  timeoutMap.put(method, methodNanos);
-               } else {
-                  timeoutMap.put(method, typeNanos);
-               }
-
                methodMap.put(method, delegatedMethod);
             } else {
                syncMethodMap.put(method, delegatedMethod);
@@ -116,6 +105,16 @@ public class SyncProxy implements InvocationHandler {
       }
    }
 
+   private Long getTimeout(Method method, long typeNanos, final Map<String,Long> timeouts) {
+      Long timeout = overrideTimeout(method, timeouts);
+      if (timeout == null && method.isAnnotationPresent(Timeout.class)) {
+         Timeout methodTimeout = method.getAnnotation(Timeout.class);
+         timeout = convertToNanos(methodTimeout);
+      }
+      return timeout != null ? timeout : typeNanos;
+
+   }
+
    static long convertToNanos(Timeout timeout) {
       long methodNanos = TimeUnit.NANOSECONDS.convert(timeout.duration(), timeout.timeUnit());
       return methodNanos;
@@ -151,26 +150,16 @@ public class SyncProxy implements InvocationHandler {
    }
 
    // override timeout by values configured in properties(in ms)
-   private void overrideTimeout(Class<?> declaring, Method method, Multimap<String, String> constants) {
+   private Long overrideTimeout(final Method method, final Map<String, Long> timeouts) {
-      if (constants == null) {
+      if (timeouts == null) {
-         return;
+         return null;
       }
-      final String classTimeouts = PROPERTY_TIMEOUTS_PREFIX + declaring.getSimpleName();
+      final String className = declaring.getSimpleName();
-      String strTimeout = Iterables.getFirst(constants.get(classTimeouts + "." + method.getName()), null);
+      Long timeout = timeouts.get(className + "." + method.getName());
-      if (strTimeout == null) {
+      if (timeout == null) {
-         strTimeout = Iterables.getFirst(constants.get(classTimeouts), null);
+         timeout = timeouts.get(className);
-      }
-      if (strTimeout != null) {
-         long timeout = 0l;
-         try {
-            timeout = Long.valueOf(strTimeout);
-         } catch (final Throwable t) {
-             timeout = 0l;
-         }
-         if (timeout > 0l) {
-            timeoutMap.put(method, TimeUnit.NANOSECONDS.convert(timeout, TimeUnit.MILLISECONDS));
-         }
       }
+      return timeout != null ? TimeUnit.MILLISECONDS.toNanos(timeout) : null;
    }
 
    @Override

core/src/main/java/org/jclouds/rest/RestContextBuilder.java

@@ -36,19 +36,17 @@ import static org.jclouds.Constants.PROPERTY_ENDPOINT;
 import static org.jclouds.Constants.PROPERTY_IDENTITY;
 import static org.jclouds.Constants.PROPERTY_ISO3166_CODES;
 import static org.jclouds.Constants.PROPERTY_PROVIDER;
+import static org.jclouds.Constants.PROPERTY_TIMEOUTS_PREFIX;
 
 import java.net.URI;
-import java.util.ArrayList;
+import java.util.*;
-import java.util.List;
-import java.util.Map;
-import java.util.Properties;
-import java.util.Set;
 import java.util.Map.Entry;
 
 import javax.inject.Inject;
 import javax.inject.Named;
 import javax.inject.Singleton;
 
+import com.google.common.collect.*;
 import org.jclouds.concurrent.MoreExecutors;
 import org.jclouds.concurrent.SingleThreaded;
 import org.jclouds.concurrent.config.ConfiguresExecutorService;
@@ -72,10 +70,6 @@ import org.jclouds.rest.internal.RestContextImpl;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Predicate;
-import com.google.common.collect.ImmutableMultimap;
-import com.google.common.collect.ImmutableSet;
-import com.google.common.collect.LinkedHashMultimap;
-import com.google.common.collect.Multimap;
 import com.google.inject.AbstractModule;
 import com.google.inject.Guice;
 import com.google.inject.Injector;
@@ -117,6 +111,23 @@ public class RestContextBuilder<S, A> {
          return LinkedHashMultimap.create(builder.build());
       }
 
+      @Provides
+      @Singleton
+      @Named("TIMEOUTS")
+      protected Map<String, Long> timeouts() {
+         final ImmutableMap.Builder<String, Long> builder = ImmutableMap.<String, Long> builder();
+         for (final Entry<Object, Object> entry : properties.entrySet()) {
+            final String key = String.valueOf(entry.getKey());
+            if (key.startsWith(PROPERTY_TIMEOUTS_PREFIX) && entry.getValue() != null) {
+               try {
+                  final Long val = Long.valueOf(String.valueOf(entry.getValue()));
+                  builder.put(key.replaceFirst(PROPERTY_TIMEOUTS_PREFIX, ""), val);
+               } catch (final Throwable t) {}
+            }
+         }
+         return builder.build();
+      }
+
       @Override
       protected void configure() {
          Properties toBind = new Properties();

core/src/main/java/org/jclouds/rest/config/ClientProvider.java

@@ -63,10 +63,9 @@ public class ClientProvider<S, A> implements Provider<S> {
                new TypeLiteral<Cache<ClassMethodArgs, Object>>() {
                }, Names.named("sync")));
       try {
-         final SyncProxy syncProxy = new SyncProxy(syncClientType, client,
+         return (S) SyncProxy.proxy(syncClientType, client, delegateMap, sync2Async,
-                  delegateMap, sync2Async);
+                 injector.getInstance(Key.get(new TypeLiteral<Map<String, Long>>() {
-         injector.injectMembers(syncProxy);
+               }, Names.named("TIMEOUTS"))));
-         return (S) SyncProxy.proxy(syncClientType, syncProxy);
       } catch (Exception e) {
          Throwables.propagate(e);
          assert false : "should have propagated";

core/src/main/java/org/jclouds/rest/config/CreateClientForCaller.java

@@ -28,6 +28,9 @@ import javax.inject.Named;
 import javax.inject.Provider;
 
 import com.google.inject.Injector;
+import com.google.inject.Key;
+import com.google.inject.TypeLiteral;
+import com.google.inject.name.Names;
 import org.jclouds.concurrent.internal.SyncProxy;
 import org.jclouds.internal.ClassMethodArgs;
 
@@ -64,10 +67,9 @@ public class CreateClientForCaller extends CacheLoader<ClassMethodArgs, Object>
       Object asyncClient = asyncMap.get(from);
       checkState(asyncClient != null, "configuration error, sync client for " + from + " not found");
       try {
-         final SyncProxy syncProxy = new SyncProxy(syncClass, asyncClient, delegateMap.get(), sync2Async);
+         return SyncProxy.proxy(syncClass, asyncClient, delegateMap.get(), sync2Async,
-         injector.injectMembers(syncProxy);
+                 injector.getInstance(Key.get(new TypeLiteral<Map<String, Long>>() {
-
+                 }, Names.named("TIMEOUTS"))));
-         return SyncProxy.proxy(syncClass, syncProxy);
       } catch (Exception e) {
          Throwables.propagate(e);
          assert false : "should have propagated";

core/src/test/java/org/jclouds/concurrent/internal/SyncProxyTest.java

@@ -29,11 +29,6 @@ import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 
-import com.google.common.collect.LinkedHashMultimap;
-import com.google.common.collect.Multimap;
-import com.google.inject.AbstractModule;
-import com.google.inject.Guice;
-import com.google.inject.Injector;
 import org.jclouds.concurrent.Futures;
 import org.jclouds.concurrent.Timeout;
 import org.jclouds.internal.ClassMethodArgs;
@@ -49,9 +44,6 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.util.concurrent.ListenableFuture;
 import com.google.inject.Provides;
 
-import javax.inject.Named;
-import javax.inject.Singleton;
-
 /**
  * Tests behavior of ListenableFutureExceptionParser
  * 
@@ -59,21 +51,6 @@ import javax.inject.Singleton;
  */
 @Test(groups = "unit", singleThreaded = true)
 public class SyncProxyTest {
-   Injector injector = Guice.createInjector(new AbstractModule() {
-       @SuppressWarnings("unused")
-       @Provides
-       @Singleton
-       @Named("CONSTANTS")
-       protected Multimap<String, String> constants() {
-           final Multimap<String, String> props = LinkedHashMultimap.create();
-           props.put("jclouds.timeouts.Sync.takeXMillisecondsPropOverride", "250");
-           return props;
-       }
-
-       @Override
-       protected void configure() {
-       }
-   });
 
    @Test
    void testConvertNanos() {
@@ -205,9 +182,8 @@ public class SyncProxyTest {
    @BeforeTest
    public void setUp() throws IllegalArgumentException, SecurityException, NoSuchMethodException {
       Cache<ClassMethodArgs, Object> cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.<Object>constant(null)));
-      final SyncProxy proxy = new SyncProxy(Sync.class, new Async(),cache, ImmutableMap.<Class<?>, Class<?>> of());
+      sync = SyncProxy.proxy(Sync.class, new Async(),cache, ImmutableMap.<Class<?>, Class<?>> of(),
-      injector.injectMembers(proxy);
+              ImmutableMap.of("Sync.takeXMillisecondsPropOverride", 250L));
-      sync = SyncProxy.proxy(Sync.class, proxy);
       // just to warm up
       sync.string();
    }
@@ -280,8 +256,8 @@ public class SyncProxyTest {
    public void testWrongTypedException() throws IllegalArgumentException, SecurityException, NoSuchMethodException,
             IOException {
       Cache<ClassMethodArgs, Object> cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.<Object>constant(null)));
-      SyncProxy.proxy(SyncWrongException.class, new SyncProxy(SyncWrongException.class, new Async(),
+      SyncProxy.proxy(SyncWrongException.class,  new Async(), cache, ImmutableMap.<Class<?>, Class<?>> of(),
-            cache, ImmutableMap.<Class<?>, Class<?>> of()));
+              ImmutableMap.<String, Long>of());
    }
 
    private static interface SyncNoTimeOut {
@@ -299,8 +275,30 @@ public class SyncProxyTest {
    public void testNoTimeOutException() throws IllegalArgumentException, SecurityException, NoSuchMethodException,
             IOException {
       Cache<ClassMethodArgs, Object> cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.<Object>constant(null)));
-      SyncProxy.proxy(SyncNoTimeOut.class, new SyncProxy(SyncNoTimeOut.class, new Async(),
+      SyncProxy.proxy(SyncNoTimeOut.class, new Async(),
-            cache, ImmutableMap.<Class<?>, Class<?>> of()));
+            cache, ImmutableMap.<Class<?>, Class<?>> of(), ImmutableMap.<String, Long>of());
    }
 
+
+   @Timeout(duration = 30, timeUnit = TimeUnit.SECONDS)
+   private static interface SyncClassOverride {
+      String getString();
+
+      String newString();
+
+      String getRuntimeException();
+
+      @Timeout(duration = 300, timeUnit = TimeUnit.MILLISECONDS)
+      String takeXMillisecondsPropOverride(long ms);
+
+   }
+
+   @Test(expectedExceptions = RuntimeException.class)
+   public void testClassOverridePropTimeout() throws Exception {
+      Cache<ClassMethodArgs, Object> cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.<Object>constant(null)));
+      final SyncClassOverride sync2 = SyncProxy.proxy(SyncClassOverride.class, new Async(),
+            cache, ImmutableMap.<Class<?>, Class<?>> of(), ImmutableMap.<String, Long>of("SyncClassOverride", 100L));
+
+      assertEquals(sync2.takeXMillisecondsPropOverride(200), "foo");
+   }
 }