Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,16 @@ public static void run() throws Exception {
restTemplate = RestTemplateBuilder.create();
controller = BeanUtils.getBean("controller");

String prefix = "cse://springmvc";

try {
// this test class is intended for rery hang issue JAV-27
restTemplate.getForObject(prefix + "/controller/sayhi?name=throwexception", String.class);
TestMgr.check("true", "false");
} catch (Exception e) {
TestMgr.check("true", "true");
}

CodeFirstRestTemplateSpringmvc codeFirstClient =
BeanUtils.getContext().getBean(CodeFirstRestTemplateSpringmvc.class);
codeFirstClient.testCodeFirst(restTemplate, "springmvc", "/codeFirstSpringmvc/");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@ cse:
tracing:
enabled: true
samplingRate: 0.5
loadbalance:
retryEnabled: true
retryOnSame: 1
retryOnNext: 1

#########SSL options
ssl.protocols: TLSv1.2
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ public String saySomething(String prefix, @RequestBody Person user) {
@RequestMapping(path = "/sayhi", method = RequestMethod.GET)
public String sayHi(HttpServletRequest request) {
String[] values = request.getParameterValues("name");
if (values != null && values.length > 0 && values[0].equals("throwexception")) {
throw new RuntimeException();
}
return "hi " + request.getParameter("name") + " " + Arrays.toString(values);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ public static <CLIENT_POOL, CLIENT_OPTIONS> DeploymentOptions createClientDeploy
return options;
}

// deploy Verticle and wait for its success. do not call this method in event-loop thread
public static <VERTICLE extends AbstractVerticle> boolean blockDeploy(Vertx vertx,
Class<VERTICLE> cls,
DeploymentOptions options) throws InterruptedException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@
import java.util.Map;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.Executor;
import java.util.concurrent.ExecutorService;
import java.util.concurrent.Executors;
import java.util.concurrent.ThreadFactory;
import java.util.concurrent.atomic.AtomicInteger;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
Expand Down Expand Up @@ -53,6 +57,15 @@
public class LoadbalanceHandler extends AbstractHandler {
private static final Logger LOGGER = LoggerFactory.getLogger(LoadbalanceHandler.class);

private static final ExecutorService RETRY_POOL = Executors.newCachedThreadPool(new ThreadFactory() {
Copy link
Member

Choose a reason for hiding this comment

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

It could be better to use fix size thread pool to avoid the resource leak.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This Is not a resource leak stuff, but a performance stuff I think. And I.think it's better to use a cached pool to reuse in high volume request and when no requests thread will be destroyed automatically

private AtomicInteger count = new AtomicInteger(0);

@Override
public Thread newThread(Runnable r) {
return new Thread(r, "retry-pool-thread-" + count.getAndIncrement());
}
});

// 会给每个Microservice创建一个handler实例,因此这里的key为transportName,保证每个通道使用一个负载均衡策略
private volatile Map<String, LoadBalancer> loadBalancerMap = new ConcurrentHashMap<>();

Expand Down Expand Up @@ -181,7 +194,9 @@ private void sendWithRetry(Invocation invocation, AsyncResponse asyncResp,
newExecutor = new Executor() {
@Override
public void execute(Runnable command) {
command.run(); // retry的场景,对于同步调用, 需要在网络线程中进行。同步调用的主线程已经被挂起,无法再主线程中进行重试。
// retry的场景,对于同步调用, 同步调用的主线程已经被挂起,无法再主线程中进行重试;
Copy link
Member

Choose a reason for hiding this comment

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

can you reproduce this issue in a test and ensure it's fixed in the new solution?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's quite easy to reproduce according to the issue discretion, and I have test it.

Copy link
Contributor Author

@liubao68 liubao68 Jun 23, 2017

Choose a reason for hiding this comment

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

start a producer to listen to both rpc/highway, and a rest api that throws an exception. start a consumer with load balance retry enabled, and call this method

Copy link
Member

Choose a reason for hiding this comment

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

great, could you please add an automated test to ensure it won't be broken by changes in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

// 重试也不能在网络线程(event-loop)中进行,未被保护的阻塞操作会导致网络线程挂起
RETRY_POOL.submit(command);
}
};
invocation.setResponseExecutor(newExecutor);
Expand Down