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]
With regards,
Apache Git Services
