Adding Method to Retryable Exception for evaluation

Closes OpenFeign#719

This change adds the original Request Method to `RetryableException`,
allowing implementers to determine if a retry should occur based on
method and exception type.

To support this, `Response` objects now require that the original
`Request` be present.  Test Cases, benchmarks, and documentation have
been added.

README.md

@@ -473,6 +473,37 @@ MyApi myApi = Feign.builder()
                  .target(MyApi.class, "https://api.hostname.com");
 ```
 
+### Error Handling
+If you need more control over handling unexpected responses, Feign instances can
+register a custom `ErrorDecoder` via the builder.
+
+```java
+MyApi myApi = Feign.builder()
+                 .errorDecoder(new MyErrorDecoder())
+                 .target(MyApi.class, "https://api.hostname.com");
+```
+
+All responses that result in an HTTP status not in the 2xx range will trigger the `ErrorDecoder`'s `decode` method, allowing
+you to handle the response, wrap the failure into a custom exception or perform any additional processing.
+If you want to retry the request again, throw a `RetryableException`.  This will invoke the registered
+`Retyer`.
+
+### Retry
+Feign, by default, will automatically retry `IOException`s, regardless of HTTP method, treating them as transient network
+related exceptions, and any `RetryableException` thrown from an `ErrorDecoder`.  To customize this
+behavior, register a custom `Retryer` instance via the builder.
+
+```java
+MyApi myApi = Feign.builder()
+                 .retryer(new MyRetryer())
+                 .target(MyApi.class, "https://api.hostname.com");
+```
+
+`Retryer`s are responsible for determining if a retry should occur by returning either a `true` or
+`false` from the method `continueOrPropagate(RetryableException e);`  A `Retryer` instance will be 
+created for each `Client` execution, allowing you to maintain state bewteen each request if desired.
+If the retry is determined to be unsucessful, the last `RetryException` will be thrown.
+
 #### Static and Default Methods
 Interfaces targeted by Feign may have static or default methods (if using Java 8+).
 These allows Feign clients to contain logic that is not expressly defined by the underlying API.

benchmark/pom.xml

@@ -66,17 +66,17 @@
     <dependency>
       <groupId>io.reactivex</groupId>
       <artifactId>rxnetty</artifactId>
-      <version>0.4.14</version>
+      <version>0.5.1</version>
     </dependency>
     <dependency>
-      <groupId>io.reactivex</groupId>
-      <artifactId>rxjava</artifactId>
-      <version>1.0.17</version>
+      <groupId>io.netty</groupId>
+      <artifactId>netty-buffer</artifactId>
+      <version>4.1.0.Beta7</version>
     </dependency>
     <dependency>
-      <groupId>io.netty</groupId>
-      <artifactId>netty-codec-http</artifactId>
-      <version>4.1.0.Beta8</version>
+      <groupId>io.reactivex</groupId>
+      <artifactId>rxjava</artifactId>
+      <version>1.0.14</version>
     </dependency>
     <dependency>
       <groupId>org.openjdk.jmh</groupId>

benchmark/src/main/java/feign/benchmark/DecoderIteratorsBenchmark.java

@@ -14,6 +14,7 @@
 package feign.benchmark;
 
 import com.fasterxml.jackson.core.type.TypeReference;
+import feign.Request;
 import feign.Response;
 import feign.Util;
 import feign.codec.Decoder;
@@ -78,6 +79,7 @@ public void buildResponse() {
     response = Response.builder()
         .status(200)
         .reason("OK")
+        .request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8))
         .headers(Collections.emptyMap())
         .body(carsJson(Integer.valueOf(size)), Util.UTF_8)
         .build();

benchmark/src/main/java/feign/benchmark/RealRequestBenchmarks.java

@@ -13,6 +13,16 @@
  */
 package feign.benchmark;
 
+import feign.Logger;
+import feign.Logger.Level;
+import feign.Retryer;
+import io.netty.buffer.ByteBuf;
+import io.reactivex.netty.RxNetty;
+import io.reactivex.netty.protocol.http.server.HttpServer;
+import io.reactivex.netty.protocol.http.server.HttpServerRequest;
+import io.reactivex.netty.protocol.http.server.HttpServerResponse;
+import io.reactivex.netty.protocol.http.server.RequestHandler;
+import io.reactivex.netty.server.ErrorHandler;
 import okhttp3.OkHttpClient;
 import okhttp3.Request;
 import org.openjdk.jmh.annotations.Benchmark;
@@ -30,12 +40,7 @@
 import java.util.concurrent.TimeUnit;
 import feign.Feign;
 import feign.Response;
-import io.netty.buffer.ByteBuf;
-import io.reactivex.netty.RxNetty;
-import io.reactivex.netty.protocol.http.server.HttpServer;
-import io.reactivex.netty.protocol.http.server.HttpServerRequest;
-import io.reactivex.netty.protocol.http.server.HttpServerResponse;
-import io.reactivex.netty.protocol.http.server.RequestHandler;
+import rx.Observable;
 
 @Measurement(iterations = 5, time = 1)
 @Warmup(iterations = 10, time = 1)
@@ -53,17 +58,15 @@ public class RealRequestBenchmarks {
 
   @Setup
   public void setup() {
-    server = RxNetty.createHttpServer(SERVER_PORT, new RequestHandler<ByteBuf, ByteBuf>() {
-      public rx.Observable handle(HttpServerRequest<ByteBuf> request,
-                                  HttpServerResponse<ByteBuf> response) {
-        return response.flush();
-      }
-    });
+    server = RxNetty.createHttpServer(SERVER_PORT, (request, response) -> response.flush());
     server.start();
     client = new OkHttpClient();
     client.retryOnConnectionFailure();
     okFeign = Feign.builder()
         .client(new feign.okhttp.OkHttpClient(client))
+        .logLevel(Level.NONE)
+        .logger(new Logger.ErrorLogger())
+        .retryer(new Retryer.Default())
         .target(FeignTestInterface.class, "http://localhost:" + SERVER_PORT);
     queryRequest = new Request.Builder()
         .url("http://localhost:" + SERVER_PORT + "/?Action=GetUser&Version=2010-05-08&limit=1")
@@ -89,7 +92,10 @@ public okhttp3.Response query_baseCaseUsingOkHttp() throws IOException {
    * How fast can we execute get commands synchronously using Feign?
    */
   @Benchmark
-  public Response query_feignUsingOkHttp() {
-    return okFeign.query();
+  public boolean query_feignUsingOkHttp() {
+    /* auto close the response */
+    try (Response ignored = okFeign.query()) {
+      return true;
+    }
   }
 }

core/src/main/java/feign/Client.java

@@ -65,7 +65,7 @@ public Default(SSLSocketFactory sslContextFactory, HostnameVerifier hostnameVeri
     @Override
     public Response execute(Request request, Options options) throws IOException {
       HttpURLConnection connection = convertAndSend(request, options);
-      return convertResponse(connection).toBuilder().request(request).build();
+      return convertResponse(connection, request).toBuilder().request(request).build();
     }
 
     HttpURLConnection convertAndSend(Request request, Options options) throws IOException {
@@ -139,7 +139,7 @@ HttpURLConnection convertAndSend(Request request, Options options) throws IOExce
       return connection;
     }
 
-    Response convertResponse(HttpURLConnection connection) throws IOException {
+    Response convertResponse(HttpURLConnection connection, Request request) throws IOException {
       int status = connection.getResponseCode();
       String reason = connection.getResponseMessage();
 
@@ -170,6 +170,7 @@ Response convertResponse(HttpURLConnection connection) throws IOException {
           .status(status)
           .reason(reason)
           .headers(headers)
+          .request(request)
           .body(stream, length)
           .build();
     }

core/src/main/java/feign/FeignException.java

@@ -13,8 +13,8 @@
  */
 package feign;
 
-import java.io.IOException;
 import static java.lang.String.format;
+import java.io.IOException;
 
 /**
  * Origin exception type for all Http Apis.
@@ -61,7 +61,9 @@ public static FeignException errorStatus(String methodKey, Response response) {
 
   static FeignException errorExecuting(Request request, IOException cause) {
     return new RetryableException(
-        format("%s executing %s %s", cause.getMessage(), request.method(), request.url()), cause,
+        format("%s executing %s %s", cause.getMessage(), request.method(), request.url()),
+        request.method(),
+        cause,
         null);
   }
 }

core/src/main/java/feign/Request.java

@@ -13,18 +13,22 @@
  */
 package feign;
 
+import static feign.Util.checkNotNull;
+import static feign.Util.valuesOrEmpty;
 import java.net.HttpURLConnection;
 import java.nio.charset.Charset;
 import java.util.Collection;
 import java.util.Map;
-import static feign.Util.checkNotNull;
-import static feign.Util.valuesOrEmpty;
 
 /**
  * An immutable request to an http server.
  */
 public final class Request {
 
+  public enum Methods {
+    GET, POST, PUT, PATCH, DELETE, OPTIONS, HEAD
+  }
+
   /**
    * No parameters can be null except {@code body} and {@code charset}. All parameters must be
    * effectively immutable, via safe copies, not mutating or otherwise.
@@ -37,15 +41,15 @@ public static Request create(String method,
     return new Request(method, url, headers, body, charset);
   }
 
-  private final String method;
+  private final Methods method;
   private final String url;
   private final Map<String, Collection<String>> headers;
   private final byte[] body;
   private final Charset charset;
 
   Request(String method, String url, Map<String, Collection<String>> headers, byte[] body,
       Charset charset) {
-    this.method = checkNotNull(method, "method of %s", url);
+    this.method = Methods.valueOf(checkNotNull(method, "method of %s", url));
     this.url = checkNotNull(url, "url");
     this.headers = checkNotNull(headers, "headers of %s %s", method, url);
     this.body = body; // nullable
@@ -54,7 +58,7 @@ public static Request create(String method,
 
   /* Method to invoke on the server. */
   public String method() {
-    return method;
+    return method.name();
   }
 
   /* Fully resolved URL including query. */

core/src/main/java/feign/RequestTemplate.java

@@ -166,7 +166,7 @@ public static String expand(String template, Map<String, ?> variables) {
   }
 
   private static Map<String, Collection<String>> parseAndDecodeQueries(String queryLine) {
-    Map<String, Collection<String>> map = new LinkedHashMap<String, Collection<String>>();
+    Map<String, Collection<String>> map = new LinkedHashMap<>();
     if (emptyToNull(queryLine) == null) {
       return map;
     }

core/src/main/java/feign/Response.java

@@ -45,11 +45,12 @@ public final class Response implements Closeable {
 
   private Response(Builder builder) {
     checkState(builder.status >= 200, "Invalid status code: %s", builder.status);
+    checkState(builder.request != null, "original request is required");
     this.status = builder.status;
+    this.request = builder.request;
     this.reason = builder.reason; // nullable
     this.headers = Collections.unmodifiableMap(caseInsensitiveCopyOf(builder.headers));
     this.body = builder.body; // nullable
-    this.request = builder.request; // nullable
   }
 
   public Builder toBuilder() {
@@ -121,12 +122,13 @@ public Builder body(String text, Charset charset) {
 
     /**
      * @see Response#request
-     *
-     *      NOTE: will add null check in version 10 which may require changes to custom feign.Client
-     *      or loggers
      */
     public Builder request(Request request) {
-      this.request = request;
+      checkNotNull(request, "the original request is required on all responses");
+
+      /* don't keep the body, we don't want to tie up memory on large requests */
+      this.request = Request.create(
+          request.method(), request.url(), request.headers(), null, request.charset());
       return this;
     }
 

core/src/main/java/feign/RetryableException.java

@@ -24,20 +24,23 @@ public class RetryableException extends FeignException {
   private static final long serialVersionUID = 1L;
 
   private final Long retryAfter;
+  private final String method;
 
   /**
    * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header.
    */
-  public RetryableException(String message, Throwable cause, Date retryAfter) {
+  public RetryableException(String message, String method, Throwable cause, Date retryAfter) {
     super(message, cause);
+    this.method = method;
     this.retryAfter = retryAfter != null ? retryAfter.getTime() : null;
   }
 
   /**
    * @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header.
    */
-  public RetryableException(String message, Date retryAfter) {
+  public RetryableException(String message, String method, Date retryAfter) {
     super(message);
+    this.method = method;
     this.retryAfter = retryAfter != null ? retryAfter.getTime() : null;
   }
 
@@ -48,4 +51,8 @@ public RetryableException(String message, Date retryAfter) {
   public Date retryAfter() {
     return retryAfter != null ? new Date(retryAfter) : null;
   }
+
+  public String method() {
+    return this.method;
+  }
 }

core/src/main/java/feign/codec/ErrorDecoder.java

@@ -93,7 +93,11 @@ public Exception decode(String methodKey, Response response) {
       FeignException exception = errorStatus(methodKey, response);
       Date retryAfter = retryAfterDecoder.apply(firstOrNull(response.headers(), RETRY_AFTER));
       if (retryAfter != null) {
-        return new RetryableException(exception.getMessage(), exception, retryAfter);
+        return new RetryableException(
+            exception.getMessage(),
+            response.request().method(),
+            exception,
+            retryAfter);
       }
       return exception;
     }

core/src/test/java/feign/FeignTest.java

@@ -483,7 +483,7 @@ public void retryableExceptionInDecoder() throws Exception {
           public Object decode(Response response, Type type) throws IOException {
             String string = super.decode(response, type).toString();
             if ("retry!".equals(string)) {
-              throw new RetryableException(string, null);
+              throw new RetryableException(string, "post", null);
             }
             return string;
           }
@@ -524,7 +524,7 @@ public void ensureRetryerClonesItself() {
         .errorDecoder(new ErrorDecoder() {
           @Override
           public Exception decode(String methodKey, Response response) {
-            return new RetryableException("play it again sam!", null);
+            return new RetryableException("play it again sam!", "post", null);
           }
         }).target(TestInterface.class, "http://localhost:" + server.getPort());
 
@@ -541,6 +541,7 @@ public void whenReturnTypeIsResponseNoErrorHandling() {
         .status(302)
         .reason("Found")
         .headers(headers)
+        .request(Request.create("GET", "/", Collections.emptyMap(), null, Util.UTF_8))
         .body(new byte[0])
         .build();
 
@@ -740,6 +741,7 @@ private Response responseWithText(String text) {
     return Response.builder()
         .body(text, Util.UTF_8)
         .status(200)
+        .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8))
         .headers(new HashMap<String, Collection<String>>())
         .build();
   }

core/src/test/java/feign/ResponseTest.java

@@ -30,6 +30,7 @@ public void reasonPhraseIsOptional() {
     Response response = Response.builder()
         .status(200)
         .headers(Collections.<String, Collection<String>>emptyMap())
+        .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8))
         .body(new byte[0])
         .build();
 
@@ -45,6 +46,7 @@ public void canAccessHeadersCaseInsensitively() {
     Response response = Response.builder()
         .status(200)
         .headers(headersMap)
+        .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8))
         .body(new byte[0])
         .build();
     assertThat(response.headers().get("content-type")).isEqualTo(valueList);
@@ -60,6 +62,7 @@ public void headerValuesWithSameNameOnlyVaryingInCaseAreMerged() {
     Response response = Response.builder()
         .status(200)
         .headers(headersMap)
+        .request(Request.create("GET", "/api", Collections.emptyMap(), null, Util.UTF_8))
         .body(new byte[0])
         .build();
 

core/src/test/java/feign/client/DefaultClientTest.java

@@ -13,6 +13,7 @@
  */
 package feign.client;
 
+import feign.FeignException;
 import java.io.IOException;
 import java.net.ProtocolException;
 import javax.net.ssl.HostnameVerifier;