[incubator-servicecomb-java-chassis] branch master updated: SCB-299 bug fix: ClientPoolManager thread binding memory leak.

[li...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

liubao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-servicecomb-java-chassis.git


The following commit(s) were added to refs/heads/master by this push:
     new ed15183  SCB-299 bug fix: ClientPoolManager thread binding memory leak.
ed15183 is described below

commit ed15183f8e0047fadadf562e1b411968a923ef3b
Author: wujimin <wu...@huawei.com>
AuthorDate: Wed Jan 24 21:40:13 2018 +0800

    SCB-299 bug fix: ClientPoolManager thread binding memory leak.
---
 .../foundation/vertx/client/ClientPoolManager.java | 27 +++++---------
 .../vertx/client/TestClientPoolManager.java        | 42 ++++++++++++++++++++++
 2 files changed, 51 insertions(+), 18 deletions(-)

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 b346947..a79b86d 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 @@ public class ClientPoolManager<CLIENT_POOL> {
 
   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 @@ public class ClientPoolManager<CLIENT_POOL> {
     // 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 3817d1b..884aecc 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 @@ package org.apache.servicecomb.foundation.vertx.client;
 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 class TestClientPoolManager {
     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
@@ -143,6 +162,29 @@ public class TestClientPoolManager {
   }
 
   @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);
     pools.add(pool);

-- 
To stop receiving notification emails like this one, please contact
liubao@apache.org.