[
https://issues.apache.org/jira/browse/SCB-503?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16447816#comment-16447816
]
ASF GitHub Bot commented on SCB-503:
------------------------------------
liubao68 closed pull request #657: [SCB-503]Fix stateful access problem and
resource leak issue related to WeighedResponseTimeRule
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/657
This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:
As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):
diff --git a/demo/demo-pojo/pojo-client/pom.xml
b/demo/demo-pojo/pojo-client/pom.xml
index 6f1fce32e..4b984f5f7 100644
--- a/demo/demo-pojo/pojo-client/pom.xml
+++ b/demo/demo-pojo/pojo-client/pom.xml
@@ -36,6 +36,11 @@
<groupId>org.apache.servicecomb</groupId>
<artifactId>provider-pojo</artifactId>
</dependency>
+ <dependency>
+ <groupId>org.apache.servicecomb</groupId>
+ <artifactId>foundation-test-scaffolding</artifactId>
+ <scope>compile</scope>
+ </dependency>
</dependencies>
<properties>
diff --git
a/demo/demo-pojo/pojo-client/src/main/java/org/apache/servicecomb/demo/pojo/client/PojoClient.java
b/demo/demo-pojo/pojo-client/src/main/java/org/apache/servicecomb/demo/pojo/client/PojoClient.java
index b697dc304..7ffd70dcd 100644
---
a/demo/demo-pojo/pojo-client/src/main/java/org/apache/servicecomb/demo/pojo/client/PojoClient.java
+++
b/demo/demo-pojo/pojo-client/src/main/java/org/apache/servicecomb/demo/pojo/client/PojoClient.java
@@ -20,6 +20,7 @@
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
+import java.util.Set;
import javax.inject.Inject;
@@ -36,6 +37,7 @@
import org.apache.servicecomb.demo.smartcare.SmartCare;
import org.apache.servicecomb.foundation.common.utils.BeanUtils;
import org.apache.servicecomb.foundation.common.utils.Log4jUtils;
+import org.apache.servicecomb.foundation.test.scaffolding.config.ArchaiusUtils;
import org.apache.servicecomb.provider.pojo.RpcReference;
import org.apache.servicecomb.swagger.invocation.context.ContextUtils;
import org.apache.servicecomb.swagger.invocation.context.InvocationContext;
@@ -95,7 +97,31 @@ public static void run() throws Exception {
testNull(testFromXml);
testNull(test);
testEmpty(test);
+ // This test case shows destroy of WeightedResponseTimeRule timer task.
after test finished will not print:
+ // "Weight adjusting job started" and thread
"NFLoadBalancer-serverWeightTimer-unknown" destroyed.
+ ArchaiusUtils.setProperty("cse.loadbalance.strategy.name",
"WeightedResponse");
testStringArray(test);
+ boolean checkerStated = false;
+ Set<Thread> allThreads = Thread.getAllStackTraces().keySet();
+ for (Thread t : allThreads) {
+ if (t.getName().equals("NFLoadBalancer-serverWeightTimer-unknown")) {
+ checkerStated = true;
+ }
+ }
+ TestMgr.check(checkerStated, true);
+
+ ArchaiusUtils.setProperty("cse.loadbalance.strategy.name", "RoundRobin");
+ testStringArray(test);
+
+ allThreads = Thread.getAllStackTraces().keySet();
+ boolean checkerDestroyed = true;
+ for (Thread t : allThreads) {
+ if (t.getName().equals("NFLoadBalancer-serverWeightTimer-unknown")) {
+ checkerDestroyed = false;
+ }
+ }
+ TestMgr.check(checkerDestroyed, true);
+
testChinese(test);
testStringHaveSpace(test);
testWrapParam(test);
diff --git
a/foundations/foundation-test-scaffolding/src/main/java/org/apache/servicecomb/foundation/test/scaffolding/config/ArchaiusUtils.java
b/foundations/foundation-test-scaffolding/src/main/java/org/apache/servicecomb/foundation/test/scaffolding/config/ArchaiusUtils.java
index 86eb5b23e..7172bb7c0 100644
---
a/foundations/foundation-test-scaffolding/src/main/java/org/apache/servicecomb/foundation/test/scaffolding/config/ArchaiusUtils.java
+++
b/foundations/foundation-test-scaffolding/src/main/java/org/apache/servicecomb/foundation/test/scaffolding/config/ArchaiusUtils.java
@@ -19,9 +19,9 @@
import java.lang.reflect.Field;
-import org.apache.commons.configuration.Configuration;
import org.springframework.util.ReflectionUtils;
+import com.netflix.config.ConcurrentCompositeConfiguration;
import com.netflix.config.ConfigurationManager;
import com.netflix.config.DynamicProperty;
import com.netflix.config.DynamicPropertyFactory;
@@ -62,7 +62,8 @@ public static void resetConfig() {
public static void setProperty(String key, Object value) {
// ensure have instance
DynamicPropertyFactory.getInstance();
- Configuration config = (Configuration)
DynamicPropertyFactory.getBackingConfigurationSource();
- config.addProperty(key, value);
+
+ ConcurrentCompositeConfiguration config =
(ConcurrentCompositeConfiguration)
DynamicPropertyFactory.getBackingConfigurationSource();
+ config.getConfiguration(0).addProperty(key, value);
}
}
diff --git
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
index 880da701b..bb50ad80f 100644
---
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
+++
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadBalancer.java
@@ -29,6 +29,7 @@
import com.netflix.loadbalancer.LoadBalancerStats;
import com.netflix.loadbalancer.Server;
import com.netflix.loadbalancer.ServerListFilter;
+import com.netflix.loadbalancer.WeightedResponseTimeRule;
/**
* 实现不包含服务器状态监测的负载均衡器。（这些职责在注册中心客户端实现）
@@ -61,6 +62,13 @@ public String getName() {
return name;
}
+ public void shutdown() {
+ // netflix components does not have a property way to shutdown
laodbalancers so we do it in a not quite elegant way.
+ if (this.rule instanceof WeightedResponseTimeRule) {
+ ((WeightedResponseTimeRule) this.rule).shutdown();
+ }
+ }
+
// every filter group has a loadBalancer instance
// serverList almost not changed for different invocation
// so every invocation will call setServerList, this is no problem
diff --git
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
index 7cff44e76..bcbbfd4e4 100644
---
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
+++
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/LoadbalanceHandler.java
@@ -98,7 +98,7 @@ public void handle(Invocation invocation, AsyncResponse
asyncResp) throws Except
if (!isRuleNotChanged) {
//配置变化，需要重新生成所有的lb实例
synchronized (lock) {
- loadBalancerMap.clear();
+ clearLoadBalancer();
}
}
this.policy = policy;
@@ -116,6 +116,13 @@ public void handle(Invocation invocation, AsyncResponse
asyncResp) throws Except
}
}
+ private void clearLoadBalancer() {
+ for (LoadBalancer loadBalancer : loadBalancerMap.values()) {
+ loadBalancer.shutdown();
+ }
+ loadBalancerMap.clear();
+ }
+
protected void setIsolationFilter(LoadBalancer lb, String microserviceName) {
final String filterName = IsolationServerListFilter.class.getName();
IsolationServerListFilter isolationListFilter = new
IsolationServerListFilter();
@@ -141,6 +148,7 @@ protected void setTransactionControlFilter(LoadBalancer lb,
String microserviceN
}
TransactionControlFilter transactionControlFilter =
(TransactionControlFilter) policyCls.newInstance();
transactionControlFilter.setLoadBalancerStats(lb.getLoadBalancerStats());
+ transactionControlFilter.setMicroserviceName(microserviceName);
lb.putFilter(filterName, transactionControlFilter);
} catch (Throwable e) {
String errMsg = "Fail to create instance of class: " + policyClsName;
diff --git
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/IsolationServerListFilter.java
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/IsolationServerListFilter.java
index b70c8b182..5d263e059 100644
---
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/IsolationServerListFilter.java
+++
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/IsolationServerListFilter.java
@@ -20,7 +20,6 @@
import java.util.ArrayList;
import java.util.List;
-import org.apache.servicecomb.core.Invocation;
import org.apache.servicecomb.loadbalance.Configuration;
import org.apache.servicecomb.loadbalance.CseServer;
import org.apache.servicecomb.loadbalance.ServerListFilterExt;
@@ -47,14 +46,8 @@
private int continuousFailureThreshold;
- private Invocation invocation;
-
private LoadBalancerStats stats;
- public void setInvocation(Invocation invocation) {
- this.invocation = invocation;
- }
-
public void setLoadBalancerStats(LoadBalancerStats stats) {
this.stats = stats;
}
@@ -73,7 +66,7 @@ public void setMicroserviceName(String microserviceName) {
@Override
public List<Server> getFilteredListOfServers(List<Server> servers) {
- if
(!Configuration.INSTANCE.isIsolationFilterOpen(invocation.getMicroserviceName()))
{
+ if (!Configuration.INSTANCE.isIsolationFilterOpen(microserviceName)) {
return servers;
}
diff --git
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/SimpleTransactionControlFilter.java
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/SimpleTransactionControlFilter.java
index dbb41f1ac..cbaac9769 100644
---
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/SimpleTransactionControlFilter.java
+++
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/SimpleTransactionControlFilter.java
@@ -37,7 +37,7 @@
public List<Server> getFilteredListOfServers(List<Server> servers) {
List<Server> filteredServers = new ArrayList<>();
Map<String, String> filterOptions =
-
Configuration.INSTANCE.getFlowsplitFilterOptions(getInvocation().getMicroserviceName());
+
Configuration.INSTANCE.getFlowsplitFilterOptions(this.microserviceName);
for (Server server : servers) {
if (allowVisit((CseServer) server, filterOptions)) {
filteredServers.add(server);
diff --git
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/TransactionControlFilter.java
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/TransactionControlFilter.java
index 0bde66bc7..2b51f760b 100644
---
a/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/TransactionControlFilter.java
+++
b/handlers/handler-loadbalance/src/main/java/org/apache/servicecomb/loadbalance/filter/TransactionControlFilter.java
@@ -17,16 +17,18 @@
package org.apache.servicecomb.loadbalance.filter;
-import org.apache.servicecomb.core.Invocation;
import org.apache.servicecomb.loadbalance.ServerListFilterExt;
import com.netflix.loadbalancer.LoadBalancerStats;
public abstract class TransactionControlFilter implements ServerListFilterExt {
+ private LoadBalancerStats stats;
- private Invocation invocation;
+ protected String microserviceName;
- private LoadBalancerStats stats;
+ public void setMicroserviceName(String microserviceName) {
+ this.microserviceName = microserviceName;
+ }
public void setLoadBalancerStats(LoadBalancerStats stats) {
this.stats = stats;
@@ -35,12 +37,4 @@ public void setLoadBalancerStats(LoadBalancerStats stats) {
public LoadBalancerStats getLoadBalancerStats() {
return stats;
}
-
- public Invocation getInvocation() {
- return invocation;
- }
-
- public void setInvocation(Invocation invocation) {
- this.invocation = invocation;
- }
}
diff --git
a/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadBalancer.java
b/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadBalancer.java
index 1ebce0e05..092bc4e4e 100644
---
a/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadBalancer.java
+++
b/handlers/handler-loadbalance/src/test/java/org/apache/servicecomb/loadbalance/TestLoadBalancer.java
@@ -23,6 +23,7 @@
import java.util.List;
import org.apache.servicecomb.foundation.test.scaffolding.config.ArchaiusUtils;
+import org.apache.servicecomb.loadbalance.filter.IsolationServerListFilter;
import
org.apache.servicecomb.loadbalance.filter.SimpleTransactionControlFilter;
import org.apache.servicecomb.loadbalance.filter.TransactionControlFilter;
import org.junit.Assert;
@@ -31,9 +32,14 @@
import com.netflix.loadbalancer.AbstractLoadBalancer.ServerGroup;
import com.netflix.loadbalancer.IRule;
+import com.netflix.loadbalancer.RandomRule;
+import com.netflix.loadbalancer.RoundRobinRule;
import com.netflix.loadbalancer.Server;
+import com.netflix.loadbalancer.WeightedResponseTimeRule;
import mockit.Deencapsulation;
+import mockit.Expectations;
+import mockit.Mocked;
public class TestLoadBalancer {
private IRule rule = Mockito.mock(IRule.class);
@@ -149,14 +155,134 @@ public void testGetAllServers() {
Mockito.when(filter.getFilteredListOfServers(servers)).thenReturn(servers);
Assert.assertEquals(servers, loadBalancer.getAllServers());
}
-
+
+ @Test
+ public void testLoadBalanceWithRoundRobinRuleAndFilter() {
+ // Robin components implementations require getReachableServers &
getServerList have the same size, we add a test case for this.
+ RoundRobinRule rule = new RoundRobinRule();
+ LoadBalancer lb = new LoadBalancer("lb1", rule, "service");
+ List<Server> servers = new ArrayList<>();
+ Server server = new Server("host1", 80);
+ server.setAlive(true);
+ Server server2 = new Server("host2", 80);
+ server2.setAlive(true);
+ servers.add(server);
+ servers.add(server2);
+ lb.setServerList(servers);
+ lb.putFilter("testFiler", new ServerListFilterExt() {
+ @Override
+ public List<Server> getFilteredListOfServers(List<Server> servers) {
+ List<Server> filteredServers = new ArrayList<>();
+ for (Server server : servers) {
+ if (server.getHost().equals("host1")) {
+ continue;
+ }
+ filteredServers.add(server);
+ }
+ return filteredServers;
+ }
+ });
+ Server s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ }
+
+ @Test
+ public void testLoadBalanceWithRandomRuleAndFilter() {
+ // Robin components implementations require getReachableServers &
getServerList have the same size, we add a test case for this.
+ RandomRule rule = new RandomRule();
+ LoadBalancer lb = new LoadBalancer("lb1", rule, "service");
+ List<Server> servers = new ArrayList<>();
+ Server server = new Server("host1", 80);
+ server.setAlive(true);
+ Server server2 = new Server("host2", 80);
+ server2.setAlive(true);
+ servers.add(server);
+ servers.add(server2);
+ lb.setServerList(servers);
+ lb.putFilter("testFiler", new ServerListFilterExt() {
+ @Override
+ public List<Server> getFilteredListOfServers(List<Server> servers) {
+ List<Server> filteredServers = new ArrayList<>();
+ for (Server server : servers) {
+ if (server.getHost().equals("host1")) {
+ continue;
+ }
+ filteredServers.add(server);
+ }
+ return filteredServers;
+ }
+ });
+ Server s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ }
+
+ @Test
+ public void testLoadBalanceWithWeightedResponseTimeRuleAndFilter(@Mocked
CseServer server,
+ @Mocked CseServer server2) {
+ // Robin components implementations require getReachableServers &
getServerList have the same size, we add a test case for this.
+ WeightedResponseTimeRule rule = new WeightedResponseTimeRule();
+ LoadBalancer lb = new LoadBalancer("lb1", rule, "service");
+ List<Server> servers = new ArrayList<>();
+
+ new Expectations() {
+ {
+ server.getHost();
+ result = "host1";
+
+ server2.isReadyToServe();
+ result = true;
+ server2.isAlive();
+ result = true;
+ server2.getHost();
+ result = "host2";
+ }
+ };
+
+ servers.add(server);
+ servers.add(server2);
+ lb.setServerList(servers);
+ SimpleTransactionControlFilter simpleFilter = new
SimpleTransactionControlFilter();
+ simpleFilter.setMicroserviceName("service");
+ IsolationServerListFilter isolationFilter = new
IsolationServerListFilter();
+ isolationFilter.setMicroserviceName("service");
+ lb.putFilter("simpleFilter", simpleFilter);
+ lb.putFilter("isolationFilter", isolationFilter);
+ lb.putFilter("testFiler", new ServerListFilterExt() {
+ @Override
+ public List<Server> getFilteredListOfServers(List<Server> servers) {
+ List<Server> filteredServers = new ArrayList<>();
+ for (Server server : servers) {
+ if (server.getHost().equals("host1")) {
+ continue;
+ }
+ filteredServers.add(server);
+ }
+ return filteredServers;
+ }
+ });
+ Server s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ s = lb.chooseServer("test");
+ Assert.assertEquals(server2, s);
+ }
+
@Test
public void testLoadBalanceWithSessionSticknessRule() {
SessionStickinessRule rule = new SessionStickinessRule();
LoadBalancer lb = new LoadBalancer("lb1", rule, "service");
Assert.assertEquals(lb.getMicroServiceName(), "service");
Assert.assertEquals("service", Deencapsulation.getField(rule,
"microserviceName"));
-
+
List<Server> servers = new ArrayList<>();
Server server = new Server("host1", 80);
server.setAlive(true);
@@ -165,18 +291,18 @@ public void testLoadBalanceWithSessionSticknessRule() {
servers.add(server);
servers.add(server2);
lb.setServerList(servers);
-
+
Server s = lb.chooseServer("test");
Assert.assertEquals(server2, s);
s = lb.chooseServer("test");
Assert.assertEquals(server2, s);
-
- long time = Deencapsulation.getField(rule, "lastAccessedTime");
+
+ long time = Deencapsulation.getField(rule, "lastAccessedTime");
Deencapsulation.setField(rule, "lastAccessedTime", time - 1000 * 10);
ArchaiusUtils.setProperty("cse.loadbalance.service.SessionStickinessRule.sessionTimeoutInSeconds",
9);
s = lb.chooseServer("test");
Assert.assertEquals(server, s);
-
+
ArchaiusUtils.setProperty("cse.loadbalance.service.SessionStickinessRule.successiveFailedTimes",
5);
lb.getLoadBalancerStats().incrementSuccessiveConnectionFailureCount(s);
lb.getLoadBalancerStats().incrementSuccessiveConnectionFailureCount(s);
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
> When using WeighedResponseTimeRule, there are some initialize and stateless
> access problems
> -------------------------------------------------------------------------------------------
>
> Key: SCB-503
> URL: https://issues.apache.org/jira/browse/SCB-503
> Project: Apache ServiceComb
> Issue Type: Bug
> Reporter: liubao
> Assignee: liubao
> Priority: Major
>
> 1. When creating WeighedResponseTimeRule, it will call loadbalance get all
> service lists. And when running, it also starts a deamon to maintain states
> and also call loadbalance get all service lists. So any state access to
> 'invocation' will cause problem.
> Any filter rules will avoid to use invocation for stateful filters.
>
> 2. when change WeighedResponseTimeRule to RoundRobin, it's deamon task is not
> destroyed and may cause resource leak if change laodbalance rule from to this
> frequently.
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
