Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support method option and add UT #1881

Merged
merged 9 commits into from
Nov 1, 2023
32 changes: 32 additions & 0 deletions core/src/main/java/feign/Request.java
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
package feign;

import static feign.Util.checkNotNull;
import static feign.Util.getThreadIdentifier;
import static feign.Util.valuesOrEmpty;
import java.io.Serializable;
import java.net.HttpURLConnection;
Expand All @@ -22,8 +23,10 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.TimeUnit;

/**
Expand Down Expand Up @@ -303,6 +306,34 @@ public static class Options {
private final long readTimeout;
private final TimeUnit readTimeoutUnit;
private final boolean followRedirects;
private final Map<String, Map<String, Options>> threadToMethodOptions;

/**
* Get an Options by methodName
*
* @param methodName it's your FeignInterface method name.
* @return method Options
*/
public Options getMethodOptions(String methodName) {
TaoJing96 marked this conversation as resolved.
Show resolved Hide resolved
Map<String, Options> methodOptions =
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>());
return methodOptions.getOrDefault(methodName, this);
}

/**
* Set methodOptions by methodKey and options
*
* @param methodName it's your FeignInterface method name.
* @param options it's the Options for this method.
*/
@Experimental
public void setMethodOptions(String methodName, Options options) {
String threadIdentifier = getThreadIdentifier();
Map<String, Request.Options> methodOptions =
threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>());
threadToMethodOptions.put(threadIdentifier, methodOptions);
methodOptions.put(methodName, options);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new field threadToMethodOptions and the methods getMethodOptions and setMethodOptions are introduced correctly. However, there's a potential issue with thread safety. The setMethodOptions method retrieves a Map from the ConcurrentHashMap and then modifies it. This operation is not atomic and can lead to data races if multiple threads call setMethodOptions concurrently. Consider synchronizing the method or using a thread-safe Map implementation.

-  public void setMethodOptions(String methodName, Options options) {
-    String threadIdentifier = getThreadIdentifier();
-    Map<String, Request.Options> methodOptions =
-        threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>());
-    threadToMethodOptions.put(threadIdentifier, methodOptions);
-    methodOptions.put(methodName, options);
-  }
+  public synchronized void setMethodOptions(String methodName, Options options) {
+    String threadIdentifier = getThreadIdentifier();
+    Map<String, Request.Options> methodOptions =
+        threadToMethodOptions.computeIfAbsent(threadIdentifier, k -> new ConcurrentHashMap<>());
+    methodOptions.put(methodName, options);
+  }

Commitable suggestion (Beta)
Suggested change
private final long readTimeout;
private final TimeUnit readTimeoutUnit;
private final boolean followRedirects;
private final Map<String, Map<String, Options>> threadToMethodOptions;
/**
* Get an Options by methodName
*
* @param methodName it's your FeignInterface method name.
* @return method Options
*/
public Options getMethodOptions(String methodName) {
Map<String, Options> methodOptions =
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>());
return methodOptions.getOrDefault(methodName, this);
}
/**
* Set methodOptions by methodKey and options
*
* @param methodName it's your FeignInterface method name.
* @param options it's the Options for this method.
*/
@Experimental
public void setMethodOptions(String methodName, Options options) {
String threadIdentifier = getThreadIdentifier();
Map<String, Request.Options> methodOptions =
threadToMethodOptions.getOrDefault(threadIdentifier, new HashMap<>());
threadToMethodOptions.put(threadIdentifier, methodOptions);
methodOptions.put(methodName, options);
}
private final long readTimeout;
private final TimeUnit readTimeoutUnit;
private final boolean followRedirects;
private final Map<String, Map<String, Options>> threadToMethodOptions;
/**
* Get an Options by methodName
*
* @param methodName it's your FeignInterface method name.
* @return method Options
*/
public Options getMethodOptions(String methodName) {
Map<String, Options> methodOptions =
threadToMethodOptions.getOrDefault(getThreadIdentifier(), new HashMap<>());
return methodOptions.getOrDefault(methodName, this);
}
/**
* Set methodOptions by methodKey and options
*
* @param methodName it's your FeignInterface method name.
* @param options it's the Options for this method.
*/
@Experimental
public synchronized void setMethodOptions(String methodName, Options options) {
String threadIdentifier = getThreadIdentifier();
Map<String, Request.Options> methodOptions =
threadToMethodOptions.computeIfAbsent(threadIdentifier, k -> new ConcurrentHashMap<>());
methodOptions.put(methodName, options);
}


/**
* Creates a new Options instance.
Expand Down Expand Up @@ -338,6 +369,7 @@ public Options(long connectTimeout, TimeUnit connectTimeoutUnit,
this.readTimeout = readTimeout;
this.readTimeoutUnit = readTimeoutUnit;
this.followRedirects = followRedirects;
this.threadToMethodOptions = new ConcurrentHashMap<>();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/feign/SynchronousMethodHandler.java
Original file line number Diff line number Diff line change
Expand Up @@ -128,13 +128,13 @@ Request targetRequest(RequestTemplate template) {

Options findOptions(Object[] argv) {
if (argv == null || argv.length == 0) {
return this.options;
return this.options.getMethodOptions(metadata.method().getName());
}
return Stream.of(argv)
.filter(Options.class::isInstance)
.map(Options.class::cast)
.findFirst()
.orElse(this.options);
.orElse(this.options.getMethodOptions(metadata.method().getName()));
}

static class Factory implements MethodHandler.Factory<Object> {
Expand Down
5 changes: 5 additions & 0 deletions core/src/main/java/feign/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -400,4 +400,9 @@ public static List<Field> allFields(Class<?> clazz) {
return fields;
}

public static String getThreadIdentifier() {
Thread currentThread = Thread.currentThread();
return currentThread.getThreadGroup() + "_" + currentThread.getName() + "_"
+ currentThread.getId();
}
}
46 changes: 46 additions & 0 deletions core/src/test/java/feign/OptionsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@
import org.junit.Test;
import org.junit.rules.ExpectedException;
import java.net.SocketTimeoutException;
import java.util.concurrent.CountDownLatch;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicReference;
import okhttp3.mockwebserver.MockResponse;
import okhttp3.mockwebserver.MockWebServer;
import static org.assertj.core.api.Assertions.assertThat;
Expand Down Expand Up @@ -87,4 +89,48 @@ public void normalResponseForChildOptionsTest() {

assertThat(api.getChildOptions(new ChildOptions(1000, 4 * 1000))).isEqualTo("foo");
}

@Test
public void socketTimeoutWithMethodOptionsTest() throws Exception {
final MockWebServer server = new MockWebServer();
server.enqueue(new MockResponse().setBody("foo").setBodyDelay(2, TimeUnit.SECONDS));
Request.Options options = new Request.Options(1000, 3000);
final OptionsInterface api = Feign.builder()
.options(options)
.target(OptionsInterface.class, server.url("/").toString());

AtomicReference<Exception> exceptionAtomicReference = new AtomicReference<>();
Thread thread = new Thread(() -> {
try {
options.setMethodOptions("get", new Request.Options(1000, 1000));
api.get();
} catch (Exception exception) {
exceptionAtomicReference.set(exception);
}
});
thread.start();
thread.join();
thrown.expect(FeignException.class);
thrown.expectCause(CoreMatchers.isA(SocketTimeoutException.class));
throw exceptionAtomicReference.get();
}

@Test
public void normalResponseWithMethodOptionsTest() throws Exception {
final MockWebServer server = new MockWebServer();
server.enqueue(new MockResponse().setBody("foo").setBodyDelay(2, TimeUnit.SECONDS));
Request.Options options = new Request.Options(1000, 1000);
final OptionsInterface api = Feign.builder()
.options(options)
.target(OptionsInterface.class, server.url("/").toString());

CountDownLatch countDownLatch = new CountDownLatch(1);
Thread thread = new Thread(() -> {
options.setMethodOptions("get", new Request.Options(1000, 3000));
api.get();
countDownLatch.countDown();
});
thread.start();
thread.join();
}
}