Adds LBClientFactory to enable caching of Ribbon LBClients

bstick12 · Adrian Cole · commit 4f6f8d4ecd9f · 2015-02-22T10:17:12.000-08:00
Before, LBClients were created for each request, which led to issues such as OpenFeign#182. Moreover, a user could not avoid using Ribbon's static factories. Adding LBClientFactory allows users to control how Ribbon resources are created.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,6 @@
 ### Version 7.3
 * Adds Request.Options support to RibbonClient
+* Adds LBClientFactory to enable caching of Ribbon LBClients
 * Updates to Ribbon 2.0-RC13
 * Updates to Jackson 2.5.1
 * Supports query parameters without values
diff --git a/ribbon/src/main/java/feign/ribbon/LBClient.java b/ribbon/src/main/java/feign/ribbon/LBClient.java
@@ -35,19 +35,21 @@
 import feign.RequestTemplate;
 import feign.Response;
 
-class LBClient
-    extends AbstractLoadBalancerAwareClient<LBClient.RibbonRequest, LBClient.RibbonResponse> {
+public final class LBClient extends
+    AbstractLoadBalancerAwareClient<LBClient.RibbonRequest, LBClient.RibbonResponse> {
 
-  private final Client delegate;
   private final int connectTimeout;
   private final int readTimeout;
   private final IClientConfig clientConfig;
 
-  LBClient(Client delegate, ILoadBalancer lb, IClientConfig clientConfig) {
+  public static LBClient create(ILoadBalancer lb, IClientConfig clientConfig) {
+    return new LBClient(lb, clientConfig);
+  }
+
+  LBClient(ILoadBalancer lb, IClientConfig clientConfig) {
     super(lb, clientConfig);
     this.setRetryHandler(RetryHandler.DEFAULT);
     this.clientConfig = clientConfig;
-    this.delegate = delegate;
     connectTimeout = clientConfig.get(CommonClientConfigKey.ConnectTimeout);
     readTimeout = clientConfig.get(CommonClientConfigKey.ReadTimeout);
   }
@@ -64,7 +66,7 @@ public RibbonResponse execute(RibbonRequest request, IClientConfig configOverrid
     } else {
       options = new Request.Options(connectTimeout, readTimeout);
     }
-    Response response = delegate.execute(request.toRequest(), options);
+    Response response = request.client().execute(request.toRequest(), options);
     return new RibbonResponse(request.getUri(), response);
   }
 
@@ -84,8 +86,10 @@ public RequestSpecificRetryHandler getRequestSpecificRetryHandler(
   static class RibbonRequest extends ClientRequest implements Cloneable {
 
     private final Request request;
+    private final Client client;
 
-    RibbonRequest(Request request, URI uri) {
+    RibbonRequest(Client client, Request request, URI uri) {
+      this.client = client;
       this.request = request;
       setUri(uri);
     }
@@ -99,8 +103,12 @@ Request toRequest() {
           .request();
     }
 
+    Client client() {
+      return client;
+    }
+
     public Object clone() {
-      return new RibbonRequest(request, getUri());
+      return new RibbonRequest(client, request, getUri());
     }
   }
 
diff --git a/ribbon/src/main/java/feign/ribbon/LBClientFactory.java b/ribbon/src/main/java/feign/ribbon/LBClientFactory.java
@@ -0,0 +1,22 @@
+package feign.ribbon;
+
+import com.netflix.client.ClientFactory;
+import com.netflix.client.config.IClientConfig;
+import com.netflix.loadbalancer.ILoadBalancer;
+
+public interface LBClientFactory {
+
+  LBClient create(String clientName);
+
+  /**
+   * Uses {@link ClientFactory} static factories from ribbon to create an LBClient.
+   */
+  public static final class Default implements LBClientFactory {
+    @Override
+    public LBClient create(String clientName) {
+      IClientConfig config = ClientFactory.getNamedConfig(clientName);
+      ILoadBalancer lb = ClientFactory.getNamedLoadBalancer(clientName);
+      return LBClient.create(lb, config);
+    }
+  }
+}
diff --git a/ribbon/src/main/java/feign/ribbon/LoadBalancingTarget.java b/ribbon/src/main/java/feign/ribbon/LoadBalancingTarget.java
@@ -103,7 +103,7 @@ public Request apply(RequestTemplate input) {
   @Override
   public boolean equals(Object obj) {
     if (obj instanceof LoadBalancingTarget) {
-      LoadBalancingTarget<?> other = (LoadBalancingTarget) obj;
+      LoadBalancingTarget<?> other = (LoadBalancingTarget<?>) obj;
       return type.equals(other.type)
              && name.equals(other.name);
     }
diff --git a/ribbon/src/main/java/feign/ribbon/RibbonClient.java b/ribbon/src/main/java/feign/ribbon/RibbonClient.java
@@ -1,11 +1,8 @@
 package feign.ribbon;
 
 import com.netflix.client.ClientException;
-import com.netflix.client.ClientFactory;
 import com.netflix.client.config.CommonClientConfigKey;
 import com.netflix.client.config.DefaultClientConfigImpl;
-import com.netflix.client.config.IClientConfig;
-import com.netflix.loadbalancer.ILoadBalancer;
 
 import java.io.IOException;
 import java.net.URI;
@@ -20,21 +17,37 @@
 import feign.Response;
 
 /**
- * RibbonClient can be used in Fiegn builder to activate smart routing and resiliency capabilities
+ * RibbonClient can be used in Feign builder to activate smart routing and resiliency capabilities
  * provided by Ribbon. Ex.
+ * 
  * <pre>
- * MyService api = Feign.builder.client(new RibbonClient()).target(MyService.class,
- * "http://myAppProd");
+ * MyService api = Feign.builder.client(RibbonClient.create()).target(MyService.class,
+ *     &quot;http://myAppProd&quot;);
  * </pre>
+ * 
  * Where {@code myAppProd} is the ribbon client name and {@code myAppProd.ribbon.listOfServers}
  * configuration is set.
  */
 public class RibbonClient implements Client {
 
   private final Client delegate;
+  private final LBClientFactory lbClientFactory;
 
+
+  public static RibbonClient create() {
+    return builder().build();
+  }
+
+  public static Builder builder() {
+    return new Builder();
+  }
+
+  /**
+   * @deprecated Use the {@link RibbonClient#create()}
+   */
+  @Deprecated
   public RibbonClient() {
-    this.delegate = new Client.Default(
+    this(new Client.Default(
         new Lazy<SSLSocketFactory>() {
           public SSLSocketFactory get() {
             return (SSLSocketFactory) SSLSocketFactory.getDefault();
@@ -45,11 +58,20 @@ public HostnameVerifier get() {
             return HttpsURLConnection.getDefaultHostnameVerifier();
           }
         }
-    );
+    ));
   }
 
+  /**
+   * @deprecated Use the {@link RibbonClient#create()}
+   */
+  @Deprecated
   public RibbonClient(Client delegate) {
+    this(delegate, new LBClientFactory.Default());
+  }
+
+  RibbonClient(Client delegate, LBClientFactory lbClientFactory) {
     this.delegate = delegate;
+    this.lbClientFactory = lbClientFactory;
   }
 
   @Override
@@ -58,7 +80,8 @@ public Response execute(Request request, Request.Options options) throws IOExcep
       URI asUri = URI.create(request.url());
       String clientName = asUri.getHost();
       URI uriWithoutHost = URI.create(request.url().replace(asUri.getHost(), ""));
-      LBClient.RibbonRequest ribbonRequest = new LBClient.RibbonRequest(request, uriWithoutHost);
+      LBClient.RibbonRequest ribbonRequest =
+          new LBClient.RibbonRequest(delegate, request, uriWithoutHost);
       return lbClient(clientName).executeWithLoadBalancer(ribbonRequest,
           new FeignOptionsClientConfig(options)).toResponse();
     } catch (ClientException e) {
@@ -70,9 +93,7 @@ public Response execute(Request request, Request.Options options) throws IOExcep
   }
 
   private LBClient lbClient(String clientName) {
-    IClientConfig config = ClientFactory.getNamedConfig(clientName);
-    ILoadBalancer lb = ClientFactory.getNamedLoadBalancer(clientName);
-    return new LBClient(delegate, lb, config);
+    return lbClientFactory.create(clientName);
   }
   
   static class FeignOptionsClientConfig extends DefaultClientConfigImpl {
@@ -94,4 +115,40 @@ public void loadDefaultValues() {
 
   }
   
+  public static final class Builder {
+
+    Builder() {
+    }
+
+    private Client delegate;
+    private LBClientFactory lbClientFactory;
+
+    public Builder delegate(Client delegate) {
+      this.delegate = delegate;
+      return this;
+    }
+
+    public Builder lbClientFactory(LBClientFactory lbClientFactory) {
+      this.lbClientFactory = lbClientFactory;
+      return this;
+    }
+
+    public RibbonClient build() {
+      return new RibbonClient(
+          delegate != null ? delegate : new Client.Default(
+              new Lazy<SSLSocketFactory>() {
+                public SSLSocketFactory get() {
+                  return (SSLSocketFactory) SSLSocketFactory.getDefault();
+                }
+              },
+              new Lazy<HostnameVerifier>() {
+                public HostnameVerifier get() {
+                  return HttpsURLConnection.getDefaultHostnameVerifier();
+                }
+              }
+          ),
+          lbClientFactory != null ? lbClientFactory : new LBClientFactory.Default()
+      );
+    }
+  }
 }
diff --git a/ribbon/src/main/java/feign/ribbon/RibbonModule.java b/ribbon/src/main/java/feign/ribbon/RibbonModule.java
@@ -36,6 +36,11 @@
 @dagger.Module(overrides = true, library = true, complete = false)
 public class RibbonModule {
 
+  @Provides
+  LBClientFactory lbClientFactory() {
+    return new LBClientFactory.Default();
+  }
+
   @Provides
   @Named("delegate")
   Client delegate(Client.Default delegate) {
@@ -44,7 +49,7 @@ Client delegate(Client.Default delegate) {
 
   @Provides
   @Singleton
-  Client httpClient(@Named("delegate") Client client) {
-    return new RibbonClient(client);
+  Client httpClient(@Named("delegate") Client client, LBClientFactory lbClientFactory) {
+    return new RibbonClient(client, lbClientFactory);
   }
 }
diff --git a/ribbon/src/test/java/feign/ribbon/LBClientFactoryTest.java b/ribbon/src/test/java/feign/ribbon/LBClientFactoryTest.java
@@ -0,0 +1,18 @@
+package feign.ribbon;
+
+import static org.junit.Assert.assertEquals;
+
+import org.junit.Test;
+
+import com.netflix.client.ClientFactory;
+
+public class LBClientFactoryTest {
+
+  @Test
+  public void testCreateLBClient() {
+    LBClientFactory.Default lbClientFactory = new LBClientFactory.Default();
+    LBClient client = lbClientFactory.create("clientName");
+    assertEquals("clientName", client.getClientName());
+    assertEquals(ClientFactory.getNamedLoadBalancer("clientName"), client.getLoadBalancer());
+  }
+}
diff --git a/ribbon/src/test/java/feign/ribbon/RibbonClientTest.java b/ribbon/src/test/java/feign/ribbon/RibbonClientTest.java
@@ -156,7 +156,7 @@ public HostnameVerifier get() {
     getConfigInstance().setProperty(serverListKey(), hostAndPort(server1.getUrl("")));
 
     TestInterface api =
-        Feign.builder().client(new RibbonClient(trustSSLSockets))
+        Feign.builder().client(RibbonClient.builder().delegate(trustSSLSockets).build())
             .target(TestInterface.class, "https://" + client());
     api.post();
     assertEquals(1, server1.getRequestCount());
@@ -170,9 +170,9 @@ public void ioExceptionRetryWithBuilder() throws IOException, InterruptedExcepti
 
     getConfigInstance().setProperty(serverListKey(), hostAndPort(server1.getUrl("")));
 
-    TestInterface api = Feign.builder().
-        client(new RibbonClient()).
-        target(TestInterface.class, "http://" + client());
+    TestInterface api =
+        Feign.builder().client(RibbonClient.create())
+            .target(TestInterface.class, "http://" + client());
 
     api.post();
 

Original file line number	Diff line number	Diff line change
`@@ -103,7 +103,7 @@ public Request apply(RequestTemplate input) {`
`103`	`103`	`@Override`
`104`	`104`	`public boolean equals(Object obj) {`
`105`	`105`	`if (obj instanceof LoadBalancingTarget) {`
`106`		`- LoadBalancingTarget<?> other = (LoadBalancingTarget) obj;`
	`106`	`+ LoadBalancingTarget<?> other = (LoadBalancingTarget<?>) obj;`
`107`	`107`	`return type.equals(other.type)`
`108`	`108`	`&& name.equals(other.name);`
`109`	`109`	`}`