From 20d9df3ad31803708b2edaa334eeb95a59bd8657 Mon Sep 17 00:00:00 2001 From: ddurnev Date: Mon, 3 Oct 2011 17:16:56 +0400 Subject: [PATCH] Allow users to override timeouts on sync interface: some refactoring, unit test added. See issue #253 --- .../concurrent/internal/SyncProxy.java | 65 ++++++++----------- .../org/jclouds/rest/RestContextBuilder.java | 29 ++++++--- .../jclouds/rest/config/ClientProvider.java | 7 +- .../rest/config/CreateClientForCaller.java | 10 +-- .../concurrent/internal/SyncProxyTest.java | 58 ++++++++--------- 5 files changed, 84 insertions(+), 85 deletions(-) diff --git a/core/src/main/java/org/jclouds/concurrent/internal/SyncProxy.java b/core/src/main/java/org/jclouds/concurrent/internal/SyncProxy.java index 5ef809c8cb..6eb6b68ae9 100644 --- a/core/src/main/java/org/jclouds/concurrent/internal/SyncProxy.java +++ b/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 proxy(Class clazz, SyncProxy proxy) throws IllegalArgumentException, SecurityException, + public static T proxy(Class clazz, Object async, + @Named("sync") Cache delegateMap, + Map, Class> sync2Async, Map 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 objectMethods = ImmutableSet.of(Object.class.getMethods()); @Inject - public void setProperties(@Named("CONSTANTS") Multimap props) { - for (final Method method : timeoutMap.keySet()) { - overrideTimeout(declaring, method, props); - } - } - - @Inject - public SyncProxy(Class declaring, Object async, - @Named("sync") Cache delegateMap, Map, Class> sync2Async) + private SyncProxy(Class declaring, Object async, + @Named("sync") Cache delegateMap, Map, + Class> sync2Async, final Map timeouts) 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)) { - Timeout methodTimeout = method.getAnnotation(Timeout.class); - long methodNanos = convertToNanos(methodTimeout); - timeoutMap.put(method, methodNanos); - } else { - timeoutMap.put(method, typeNanos); - } - + timeoutMap.put(method, getTimeout(method, typeNanos, timeouts)); 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 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 constants) { - if (constants == null) { - return; + private Long overrideTimeout(final Method method, final Map timeouts) { + if (timeouts == null) { + return null; } - final String classTimeouts = PROPERTY_TIMEOUTS_PREFIX + declaring.getSimpleName(); - String strTimeout = Iterables.getFirst(constants.get(classTimeouts + "." + method.getName()), null); - if (strTimeout == null) { - strTimeout = Iterables.getFirst(constants.get(classTimeouts), null); - } - 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)); - } + final String className = declaring.getSimpleName(); + Long timeout = timeouts.get(className + "." + method.getName()); + if (timeout == null) { + timeout = timeouts.get(className); } + return timeout != null ? TimeUnit.MILLISECONDS.toNanos(timeout) : null; } @Override diff --git a/core/src/main/java/org/jclouds/rest/RestContextBuilder.java b/core/src/main/java/org/jclouds/rest/RestContextBuilder.java index 6df2524fa2..94894c6956 100644 --- a/core/src/main/java/org/jclouds/rest/RestContextBuilder.java +++ b/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.List; -import java.util.Map; -import java.util.Properties; -import java.util.Set; +import java.util.*; 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 { return LinkedHashMultimap.create(builder.build()); } + @Provides + @Singleton + @Named("TIMEOUTS") + protected Map timeouts() { + final ImmutableMap.Builder builder = ImmutableMap. builder(); + for (final Entry 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(); diff --git a/core/src/main/java/org/jclouds/rest/config/ClientProvider.java b/core/src/main/java/org/jclouds/rest/config/ClientProvider.java index 4f27a0ba11..b89a3897b9 100644 --- a/core/src/main/java/org/jclouds/rest/config/ClientProvider.java +++ b/core/src/main/java/org/jclouds/rest/config/ClientProvider.java @@ -63,10 +63,9 @@ public class ClientProvider implements Provider { new TypeLiteral>() { }, Names.named("sync"))); try { - final SyncProxy syncProxy = new SyncProxy(syncClientType, client, - delegateMap, sync2Async); - injector.injectMembers(syncProxy); - return (S) SyncProxy.proxy(syncClientType, syncProxy); + return (S) SyncProxy.proxy(syncClientType, client, delegateMap, sync2Async, + injector.getInstance(Key.get(new TypeLiteral>() { + }, Names.named("TIMEOUTS")))); } catch (Exception e) { Throwables.propagate(e); assert false : "should have propagated"; diff --git a/core/src/main/java/org/jclouds/rest/config/CreateClientForCaller.java b/core/src/main/java/org/jclouds/rest/config/CreateClientForCaller.java index ea89877b8a..761ed6cb44 100644 --- a/core/src/main/java/org/jclouds/rest/config/CreateClientForCaller.java +++ b/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 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); - injector.injectMembers(syncProxy); - - return SyncProxy.proxy(syncClass, syncProxy); + return SyncProxy.proxy(syncClass, asyncClient, delegateMap.get(), sync2Async, + injector.getInstance(Key.get(new TypeLiteral>() { + }, Names.named("TIMEOUTS")))); } catch (Exception e) { Throwables.propagate(e); assert false : "should have propagated"; diff --git a/core/src/test/java/org/jclouds/concurrent/internal/SyncProxyTest.java b/core/src/test/java/org/jclouds/concurrent/internal/SyncProxyTest.java index 2806ea6d64..5a1d2b3f03 100644 --- a/core/src/test/java/org/jclouds/concurrent/internal/SyncProxyTest.java +++ b/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 constants() { - final Multimap 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 cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.constant(null))); - final SyncProxy proxy = new SyncProxy(Sync.class, new Async(),cache, ImmutableMap., Class> of()); - injector.injectMembers(proxy); - sync = SyncProxy.proxy(Sync.class, proxy); + sync = SyncProxy.proxy(Sync.class, new Async(),cache, ImmutableMap., Class> of(), + ImmutableMap.of("Sync.takeXMillisecondsPropOverride", 250L)); // just to warm up sync.string(); } @@ -280,8 +256,8 @@ public class SyncProxyTest { public void testWrongTypedException() throws IllegalArgumentException, SecurityException, NoSuchMethodException, IOException { Cache cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.constant(null))); - SyncProxy.proxy(SyncWrongException.class, new SyncProxy(SyncWrongException.class, new Async(), - cache, ImmutableMap., Class> of())); + SyncProxy.proxy(SyncWrongException.class, new Async(), cache, ImmutableMap., Class> of(), + ImmutableMap.of()); } private static interface SyncNoTimeOut { @@ -299,8 +275,30 @@ public class SyncProxyTest { public void testNoTimeOutException() throws IllegalArgumentException, SecurityException, NoSuchMethodException, IOException { Cache cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.constant(null))); - SyncProxy.proxy(SyncNoTimeOut.class, new SyncProxy(SyncNoTimeOut.class, new Async(), - cache, ImmutableMap., Class> of())); + SyncProxy.proxy(SyncNoTimeOut.class, new Async(), + cache, ImmutableMap., Class> of(), ImmutableMap.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 cache = CacheBuilder.newBuilder().build(CacheLoader.from(Functions.constant(null))); + final SyncClassOverride sync2 = SyncProxy.proxy(SyncClassOverride.class, new Async(), + cache, ImmutableMap., Class> of(), ImmutableMap.of("SyncClassOverride", 100L)); + + assertEquals(sync2.takeXMillisecondsPropOverride(200), "foo"); + } }