Skip to content

Commit

Permalink
Retryer: replace an instance of Date with an epoch millisecond (#2170)
Browse files Browse the repository at this point in the history 
* Retryer: replace an instance of Date with an epoch millisecond

* Style issue: unnecessary explicit casting

* Add another check to RetryableException's test

* Update serialization ID. Resolve some deprecation issues of Integer.

* Remove obsolete Date

* Remove obsolete Date 2

* Resolve issue with overrided method of a mock class
  • Loading branch information
@vitalijr2
vitalijr2 authored Sep 15, 2023
1 parent c7ca6e0 commit be25759
Show file tree
Hide file tree
Showing 24 changed files with 415 additions and 224 deletions.
3 changes: 2 additions & 1 deletion core/src/main/java/feign/FeignException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,13 @@ private static FeignServerException serverErrorStatus(int status,
}

static FeignException errorExecuting(Request request, IOException cause) {
final Long nonRetryable = null;
return new RetryableException(
-1,
format("%s executing %s %s", cause.getMessage(), request.httpMethod(), request.url()),
request.httpMethod(),
cause,
null, request);
nonRetryable, request);
}

public static class FeignClientException extends FeignException {
Expand Down
31 changes: 28 additions & 3 deletions core/src/main/java/feign/RetryableException.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,22 @@
*/
public class RetryableException extends FeignException {

private static final long serialVersionUID = 1L;
private static final long serialVersionUID = 2L;

private final Long retryAfter;
private final HttpMethod httpMethod;

/**
* @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header.
*/
public RetryableException(int status, String message, HttpMethod httpMethod, Throwable cause,
Long retryAfter, Request request) {
super(status, message, request, cause);
this.httpMethod = httpMethod;
this.retryAfter = retryAfter;
}

@Deprecated
public RetryableException(int status, String message, HttpMethod httpMethod, Throwable cause,
Date retryAfter, Request request) {
super(status, message, request, cause);
Expand All @@ -42,6 +50,14 @@ public RetryableException(int status, String message, HttpMethod httpMethod, Thr
/**
* @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header.
*/
public RetryableException(int status, String message, HttpMethod httpMethod, Long retryAfter,
Request request) {
super(status, message, request);
this.httpMethod = httpMethod;
this.retryAfter = retryAfter;
}

@Deprecated
public RetryableException(int status, String message, HttpMethod httpMethod, Date retryAfter,
Request request) {
super(status, message, request);
Expand All @@ -52,6 +68,15 @@ public RetryableException(int status, String message, HttpMethod httpMethod, Dat
/**
* @param retryAfter usually corresponds to the {@link feign.Util#RETRY_AFTER} header.
*/
public RetryableException(int status, String message, HttpMethod httpMethod, Long retryAfter,
Request request, byte[] responseBody,
Map<String, Collection<String>> responseHeaders) {
super(status, message, request, responseBody, responseHeaders);
this.httpMethod = httpMethod;
this.retryAfter = retryAfter;
}

@Deprecated
public RetryableException(int status, String message, HttpMethod httpMethod, Date retryAfter,
Request request, byte[] responseBody, Map<String, Collection<String>> responseHeaders) {
super(status, message, request, responseBody, responseHeaders);
Expand All @@ -63,8 +88,8 @@ public RetryableException(int status, String message, HttpMethod httpMethod, Dat
* Sometimes corresponds to the {@link feign.Util#RETRY_AFTER} header present in {@code 503}
* status. Other times parsed from an application-specific response. Null if unknown.
*/
public Date retryAfter() {
return retryAfter != null ? new Date(retryAfter) : null;
public Long retryAfter() {
return retryAfter;
}

public HttpMethod method() {
Expand Down
8 changes: 4 additions & 4 deletions core/src/main/java/feign/Retryer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
public interface Retryer extends Cloneable {

/**
* if retry is permitted, return (possibly after sleeping). Otherwise propagate the exception.
* if retry is permitted, return (possibly after sleeping). Otherwise, propagate the exception.
*/
void continueOrPropagate(RetryableException e);

Expand Down Expand Up @@ -59,7 +59,7 @@ public void continueOrPropagate(RetryableException e) {

long interval;
if (e.retryAfter() != null) {
interval = e.retryAfter().getTime() - currentTimeMillis();
interval = e.retryAfter() - currentTimeMillis();
if (interval > maxPeriod) {
interval = maxPeriod;
}
Expand All @@ -79,15 +79,15 @@ public void continueOrPropagate(RetryableException e) {
}

/**
* Calculates the time interval to a retry attempt. <br>
* Calculates the time interval to a retry attempt.<br>
* The interval increases exponentially with each attempt, at a rate of nextInterval *= 1.5
* (where 1.5 is the backoff factor), to the maximum interval.
*
* @return time in milliseconds from now until the next attempt.
*/
long nextMaxInterval() {
long interval = (long) (period * Math.pow(1.5, attempt - 1));
return interval > maxPeriod ? maxPeriod : interval;
return Math.min(interval, maxPeriod);
}

@Override
Expand Down
39 changes: 17 additions & 22 deletions core/src/main/java/feign/codec/ErrorDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,16 +16,15 @@
import static feign.FeignException.errorStatus;
import static feign.Util.RETRY_AFTER;
import static feign.Util.checkNotNull;
import static java.util.Locale.US;
import static java.time.format.DateTimeFormatter.RFC_1123_DATE_TIME;
import static java.util.concurrent.TimeUnit.SECONDS;
import feign.FeignException;
import feign.Response;
import feign.RetryableException;
import java.text.DateFormat;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.time.ZonedDateTime;
import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
import java.util.Collection;
import java.util.Date;
import java.util.Map;

/**
Expand Down Expand Up @@ -103,7 +102,7 @@ public Default(Integer maxBodyBytesLength, Integer maxBodyCharsLength) {
public Exception decode(String methodKey, Response response) {
FeignException exception = errorStatus(methodKey, response, maxBodyBytesLength,
maxBodyCharsLength);
Date retryAfter = retryAfterDecoder.apply(firstOrNull(response.headers(), RETRY_AFTER));
Long retryAfter = retryAfterDecoder.apply(firstOrNull(response.headers(), RETRY_AFTER));
if (retryAfter != null) {
return new RetryableException(
response.status(),
Expand All @@ -125,48 +124,44 @@ private <T> T firstOrNull(Map<String, Collection<T>> map, String key) {
}

/**
* Decodes a {@link feign.Util#RETRY_AFTER} header into an absolute date, if possible. <br>
* Decodes a {@link feign.Util#RETRY_AFTER} header into an epoch millisecond, if possible.<br>
* See <a href="https://tools.ietf.org/html/rfc2616#section-14.37">Retry-After format</a>
*/
static class RetryAfterDecoder {

static final DateFormat RFC822_FORMAT =
new SimpleDateFormat("EEE, dd MMM yyyy HH:mm:ss 'GMT'", US);
private final DateFormat rfc822Format;
private final DateTimeFormatter dateTimeFormatter;

RetryAfterDecoder() {
this(RFC822_FORMAT);
this(RFC_1123_DATE_TIME);
}

RetryAfterDecoder(DateFormat rfc822Format) {
this.rfc822Format = checkNotNull(rfc822Format, "rfc822Format");
RetryAfterDecoder(DateTimeFormatter dateTimeFormatter) {
this.dateTimeFormatter = checkNotNull(dateTimeFormatter, "dateTimeFormatter");
}

protected long currentTimeMillis() {
return System.currentTimeMillis();
}

/**
* returns a date that corresponds to the first time a request can be retried.
* returns an epoch millisecond that corresponds to the first time a request can be retried.
*
* @param retryAfter String in
* <a href="https://tools.ietf.org/html/rfc2616#section-14.37" >Retry-After format</a>
*/
public Date apply(String retryAfter) {
public Long apply(String retryAfter) {
if (retryAfter == null) {
return null;
}
if (retryAfter.matches("^[0-9]+\\.?0*$")) {
retryAfter = retryAfter.replaceAll("\\.0*$", "");
long deltaMillis = SECONDS.toMillis(Long.parseLong(retryAfter));
return new Date(currentTimeMillis() + deltaMillis);
return currentTimeMillis() + deltaMillis;
}
synchronized (rfc822Format) {
try {
return rfc822Format.parse(retryAfter);
} catch (ParseException ignored) {
return null;
}
try {
return ZonedDateTime.parse(retryAfter, dateTimeFormatter).toInstant().toEpochMilli();
} catch (NullPointerException | DateTimeParseException ignored) {
return null;
}
}
}
Expand Down
76 changes: 51 additions & 25 deletions core/src/test/java/feign/AsyncFeignTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import static feign.ExceptionPropagationPolicy.UNWRAP;
import static feign.Util.UTF_8;
import static feign.assertj.MockWebServerAssertions.assertThat;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.data.MapEntry.entry;
import static org.hamcrest.CoreMatchers.isA;
import static org.junit.Assert.assertEquals;
Expand All @@ -37,11 +36,13 @@
import java.io.IOException;
import java.lang.reflect.Type;
import java.net.URI;
import java.time.Clock;
import java.time.Instant;
import java.time.ZoneId;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.Date;
import java.util.HashMap;
import java.util.LinkedHashMap;
import java.util.List;
Expand All @@ -62,12 +63,12 @@
import org.junit.Ignore;
import org.junit.Rule;
import org.junit.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
import org.junit.rules.ExpectedException;

public class AsyncFeignTest {

private static final Long NON_RETRYABLE = null;
@Rule
public final ExpectedException thrown = ExpectedException.none();
@Rule
Expand Down Expand Up @@ -226,9 +227,9 @@ public void customExpander() throws Exception {
TestInterfaceAsync api =
new TestInterfaceAsyncBuilder().target("http://localhost:" + server.getPort());

CompletableFuture<?> cf = api.expand(new Date(1234l));
CompletableFuture<?> cf = api.expand(new TestClock(1234l) {});

assertThat(server.takeRequest()).hasPath("/?date=1234");
assertThat(server.takeRequest()).hasPath("/?clock=1234");

checkCFCompletedSoon(cf);
}
Expand All @@ -240,9 +241,10 @@ public void customExpanderListParam() throws Exception {
TestInterfaceAsync api =
new TestInterfaceAsyncBuilder().target("http://localhost:" + server.getPort());

CompletableFuture<?> cf = api.expandList(Arrays.asList(new Date(1234l), new Date(12345l)));
CompletableFuture<?> cf =
api.expandList(Arrays.asList(new TestClock(1234l), new TestClock(12345l)));

assertThat(server.takeRequest()).hasPath("/?date=1234&date=12345");
assertThat(server.takeRequest()).hasPath("/?clock=1234&clock=12345");

checkCFCompletedSoon(cf);
}
Expand All @@ -254,9 +256,9 @@ public void customExpanderNullParam() throws Exception {
TestInterfaceAsync api =
new TestInterfaceAsyncBuilder().target("http://localhost:" + server.getPort());

CompletableFuture<?> cf = api.expandList(Arrays.asList(new Date(1234l), null));
CompletableFuture<?> cf = api.expandList(Arrays.asList(new TestClock(1234l), null));

assertThat(server.takeRequest()).hasPath("/?date=1234");
assertThat(server.takeRequest()).hasPath("/?clock=1234");

checkCFCompletedSoon(cf);
}
Expand Down Expand Up @@ -505,8 +507,8 @@ 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(response.status(), string, HttpMethod.POST, null,
response.request());
throw new RetryableException(response.status(), string, HttpMethod.POST,
NON_RETRYABLE, response.request());
}
return string;
}
Expand Down Expand Up @@ -584,7 +586,7 @@ public void ensureRetryerClonesItself() throws Throwable {
@Override
public Exception decode(String methodKey, Response response) {
return new RetryableException(response.status(), "play it again sam!", HttpMethod.POST,
null, response.request());
NON_RETRYABLE, response.request());
}
}).target(TestInterfaceAsync.class, "http://localhost:" + server.getPort());

Expand All @@ -609,7 +611,7 @@ public void throwsOriginalExceptionAfterFailedRetries() throws Throwable {
@Override
public Exception decode(String methodKey, Response response) {
return new RetryableException(response.status(), "play it again sam!", HttpMethod.POST,
new TestInterfaceException(message), null, response.request());
new TestInterfaceException(message), NON_RETRYABLE, response.request());
}
}).target(TestInterfaceAsync.class, "http://localhost:" + server.getPort());

Expand All @@ -631,8 +633,8 @@ public void throwsRetryableExceptionIfNoUnderlyingCause() throws Throwable {
.errorDecoder(new ErrorDecoder() {
@Override
public Exception decode(String methodKey, Response response) {
return new RetryableException(response.status(), message, HttpMethod.POST, null,
response.request());
return new RetryableException(response.status(), message, HttpMethod.POST,
NON_RETRYABLE, response.request());
}
}).target(TestInterfaceAsync.class, "http://localhost:" + server.getPort());

Expand Down Expand Up @@ -1022,16 +1024,17 @@ CompletableFuture<Response> uriParam(@Param("1") String one,
CompletableFuture<Response> queryParams(@Param("1") String one,
@Param("2") Iterable<String> twos);

@RequestLine("POST /?date={date}")
CompletableFuture<Void> expand(@Param(value = "date", expander = DateToMillis.class) Date date);
@RequestLine("POST /?clock={clock}")
CompletableFuture<Void> expand(@Param(value = "clock",
expander = ClockToMillis.class) Clock clock);

@RequestLine("GET /?date={date}")
CompletableFuture<Void> expandList(@Param(value = "date",
expander = DateToMillis.class) List<Date> dates);
@RequestLine("GET /?clock={clock}")
CompletableFuture<Void> expandList(@Param(value = "clock",
expander = ClockToMillis.class) List<Clock> clocks);

@RequestLine("GET /?date={date}")
CompletableFuture<Void> expandArray(@Param(value = "date",
expander = DateToMillis.class) Date[] dates);
@RequestLine("GET /?clock={clock}")
CompletableFuture<Void> expandArray(@Param(value = "clock",
expander = ClockToMillis.class) Clock[] clocks);

@RequestLine("GET /")
CompletableFuture<Void> headerMap(@HeaderMap Map<String, Object> headerMap);
Expand Down Expand Up @@ -1062,11 +1065,11 @@ CompletableFuture<Void> queryMapWithQueryParams(@Param("name") String name,
@RequestLine("GET /")
CompletableFuture<Void> queryMapPropertyInheritence(@QueryMap ChildPojo object);

class DateToMillis implements Param.Expander {
class ClockToMillis implements Param.Expander {

@Override
public String expand(Object value) {
return String.valueOf(((Date) value).getTime());
return String.valueOf(((Clock) value).millis());
}
}
}
Expand Down Expand Up @@ -1249,5 +1252,28 @@ static interface WildApi {
CompletableFuture<?> x();
}

class TestClock extends Clock {

private long millis;

public TestClock(long millis) {
this.millis = millis;
}

@Override
public ZoneId getZone() {
throw new UnsupportedOperationException("This operation is not supported.");
}

@Override
public Clock withZone(ZoneId zone) {
return this;
}

@Override
public Instant instant() {
return Instant.ofEpochMilli(millis);
}
}

}
Loading

0 comments on commit be25759

Please sign in to comment.