[
https://issues.apache.org/jira/browse/SCB-299?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16340386#comment-16340386
]
ASF GitHub Bot commented on SCB-299:
------------------------------------
liubao68 closed pull request #528: [SCB-299] bug fix: ClientPoolManager thread
binding memory leak.
URL: https://github.com/apache/incubator-servicecomb-java-chassis/pull/528
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/foundations/foundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/client/ClientPoolManager.java
b/foundations/foundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/client/ClientPoolManager.java
index b3469478a..a79b86d7d 100644
---
a/foundations/foundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/client/ClientPoolManager.java
+++
b/foundations/foundation-vertx/src/main/java/org/apache/servicecomb/foundation/vertx/client/ClientPoolManager.java
@@ -18,13 +18,10 @@
package org.apache.servicecomb.foundation.vertx.client;
import java.util.List;
-import java.util.Map;
import java.util.UUID;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.atomic.AtomicInteger;
-import org.apache.servicecomb.foundation.common.concurrent.ConcurrentHashMapEx;
-
import io.vertx.core.Context;
import io.vertx.core.Vertx;
@@ -50,12 +47,9 @@
private List<CLIENT_POOL> pools = new CopyOnWriteArrayList<>();
- private AtomicInteger bindIndex = new AtomicInteger();
-
- // send的调用线程与CLIENT_POOL的绑定关系，不直接用hash，是担心分配不均
- // key是调用者的线程id
- // TODO:要不要考虑已经绑定的线程消失了的场景？
- private Map<Long, CLIENT_POOL> threadBindMap = new ConcurrentHashMapEx<>();
+ // reactive mode, when call from other thread, must select a context for it
+ // if we use threadId to hash a context, will always select the same context
from one thread
+ private AtomicInteger reactiveNextIndex = new AtomicInteger();
public ClientPoolManager(Vertx vertx, ClientPoolFactory<CLIENT_POOL>
factory) {
this.vertx = vertx;
@@ -103,18 +97,15 @@ protected CLIENT_POOL findByContext() {
// 2.vertx worker thread
// 3.other vertx thread
// select a existing context
- return nextPool();
+ int idx = reactiveNextIndex.getAndIncrement() % pools.size();
+ if (idx < 0) {
+ idx = -idx;
+ }
+ return pools.get(idx);
}
public CLIENT_POOL findThreadBindClientPool() {
- long threadId = Thread.currentThread().getId();
- return threadBindMap.computeIfAbsent(threadId, tid -> {
- return nextPool();
- });
- }
-
- protected CLIENT_POOL nextPool() {
- int idx = bindIndex.getAndIncrement() % pools.size();
+ int idx = (int) (Thread.currentThread().getId() % pools.size());
return pools.get(idx);
}
}
diff --git
a/foundations/foundation-vertx/src/test/java/org/apache/servicecomb/foundation/vertx/client/TestClientPoolManager.java
b/foundations/foundation-vertx/src/test/java/org/apache/servicecomb/foundation/vertx/client/TestClientPoolManager.java
index 3817d1bd0..884aecc59 100644
---
a/foundations/foundation-vertx/src/test/java/org/apache/servicecomb/foundation/vertx/client/TestClientPoolManager.java
+++
b/foundations/foundation-vertx/src/test/java/org/apache/servicecomb/foundation/vertx/client/TestClientPoolManager.java
@@ -19,6 +19,7 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
import
org.apache.servicecomb.foundation.vertx.client.http.HttpClientWithContext;
import org.hamcrest.Matchers;
@@ -117,9 +118,27 @@ public void findThreadBindClientPool(@Mocked
HttpClientWithContext pool1, @Mocke
pools.add(pool1);
pools.add(pool2);
+ new MockUp<Thread>() {
+ @Mock
+ long getId() {
+ return 0;
+ }
+ };
+
Assert.assertSame(pool1, poolMgr.findThreadBindClientPool());
// find again, get the same result
Assert.assertSame(pool1, poolMgr.findThreadBindClientPool());
+
+ new MockUp<Thread>() {
+ @Mock
+ long getId() {
+ return 1;
+ }
+ };
+
+ Assert.assertSame(pool2, poolMgr.findThreadBindClientPool());
+ // find again, get the same result
+ Assert.assertSame(pool2, poolMgr.findThreadBindClientPool());
}
@Test
@@ -142,6 +161,29 @@ public void findByContext_reactive() {
Assert.assertSame(result, poolMgr.findByContext());
}
+ @Test
+ public void findByContext_wrongContext_reverse() {
+ HttpClientWithContext pool1 = new HttpClientWithContext(null, null);
+ HttpClientWithContext pool2 = new HttpClientWithContext(null, null);
+ pools.add(pool1);
+ pools.add(pool2);
+
+ new Expectations(VertxImpl.class) {
+ {
+ VertxImpl.context();
+ result = null;
+ }
+ };
+
+ AtomicInteger reactiveNextIndex = Deencapsulation.getField(poolMgr,
"reactiveNextIndex");
+ reactiveNextIndex.set(Integer.MAX_VALUE);
+ // each time invoke find, reactiveNextIndex will inc 1
+ Assert.assertSame(pool2, poolMgr.findByContext());
+ Assert.assertSame(pool1, poolMgr.findByContext());
+ Assert.assertSame(pool2, poolMgr.findByContext());
+ Assert.assertSame(pool1, poolMgr.findByContext());
+ }
+
@Test
public void findByContext_normalThread() {
HttpClientWithContext pool = new HttpClientWithContext(null, null);
----------------------------------------------------------------
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]
> ClientPoolManager save thread binding information to map, will cause memory
> leak when using dynamic thread pool
> ---------------------------------------------------------------------------------------------------------------
>
> Key: SCB-299
> URL: https://issues.apache.org/jira/browse/SCB-299
> Project: Apache ServiceComb
> Issue Type: Bug
> Components: Java-Chassis
> Reporter: wujimin
> Assignee: wujimin
> Priority: Major
> Fix For: java-chassis-1.0.0-m1
>
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
