From 69eb4bca9f2b2f7e5728e7d8d4f12107c7b2de05 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Tue, 31 Aug 2021 17:56:45 -0700 Subject: [PATCH 1/6] add regression test --- .../io/grpc/xds/SharedCallCounterMapTest.java | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/xds/src/test/java/io/grpc/xds/SharedCallCounterMapTest.java b/xds/src/test/java/io/grpc/xds/SharedCallCounterMapTest.java index 9f2293d3c53..3051a021870 100644 --- a/xds/src/test/java/io/grpc/xds/SharedCallCounterMapTest.java +++ b/xds/src/test/java/io/grpc/xds/SharedCallCounterMapTest.java @@ -62,4 +62,22 @@ public boolean isDone() { map.cleanQueue(); assertThat(counters).isEmpty(); } + + @Test + public void gcAndRecreate() { + @SuppressWarnings("UnusedVariable") // assign to null for GC only + AtomicLong counter = map.getOrCreate(CLUSTER, EDS_SERVICE_NAME); + final CounterReference ref = counters.get(CLUSTER).get(EDS_SERVICE_NAME); + assertThat(counter.get()).isEqualTo(0); + counter = null; + GcFinalization.awaitDone(new FinalizationPredicate() { + @Override + public boolean isDone() { + return ref.isEnqueued(); + } + }); + map.getOrCreate(CLUSTER, EDS_SERVICE_NAME); + assertThat(counters.get(CLUSTER)).isNotNull(); + assertThat(counters.get(CLUSTER).get(EDS_SERVICE_NAME)).isNotNull(); + } } From d46dd18f7872cbd32d47f86ba0eeaea9b77cc9bd Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Tue, 31 Aug 2021 18:31:32 -0700 Subject: [PATCH 2/6] fix bug --- xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java index 71cff0cedf3..968f48550ac 100644 --- a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java +++ b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java @@ -52,6 +52,7 @@ static SharedCallCounterMap getInstance() { @Override public synchronized AtomicLong getOrCreate(String cluster, @Nullable String edsServiceName) { + cleanQueue(); Map clusterCounters = counters.get(cluster); if (clusterCounters == null) { clusterCounters = new HashMap<>(); @@ -64,7 +65,6 @@ public synchronized AtomicLong getOrCreate(String cluster, @Nullable String edsS ref = new CounterReference(counter, refQueue, cluster, edsServiceName); clusterCounters.put(edsServiceName, ref); } - cleanQueue(); return counter; } From 50c637f08f9e1beb5e15a4a4d5521bef67c6a1f0 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Tue, 31 Aug 2021 21:31:57 -0700 Subject: [PATCH 3/6] fix wrong fix --- .../io/grpc/xds/SharedCallCounterMap.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java index 968f48550ac..a5b323d8255 100644 --- a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java +++ b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java @@ -52,19 +52,28 @@ static SharedCallCounterMap getInstance() { @Override public synchronized AtomicLong getOrCreate(String cluster, @Nullable String edsServiceName) { - cleanQueue(); + AtomicLong counter = null; Map clusterCounters = counters.get(cluster); + if (clusterCounters != null) { + CounterReference ref = clusterCounters.get(edsServiceName); + if (ref != null) { + counter = ref.get(); + } + } + // In the case of ref.get() == null, must call cleanQueue() prior to creating a new map entry, + // otherwise the new entry will be deleted be any cleanQueue(). + cleanQueue(); + if (counter != null) { + return counter; + } + clusterCounters = counters.get(cluster); if (clusterCounters == null) { clusterCounters = new HashMap<>(); counters.put(cluster, clusterCounters); } - CounterReference ref = clusterCounters.get(edsServiceName); - AtomicLong counter; - if (ref == null || (counter = ref.get()) == null) { - counter = new AtomicLong(); - ref = new CounterReference(counter, refQueue, cluster, edsServiceName); - clusterCounters.put(edsServiceName, ref); - } + counter = new AtomicLong(); + CounterReference ref = new CounterReference(counter, refQueue, cluster, edsServiceName); + clusterCounters.put(edsServiceName, ref); return counter; } From 06cb8483f9e4c71cee18d6153527f1dff6943b75 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Tue, 31 Aug 2021 21:46:10 -0700 Subject: [PATCH 4/6] fix comment --- xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java index a5b323d8255..3011e5ec913 100644 --- a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java +++ b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java @@ -61,7 +61,7 @@ public synchronized AtomicLong getOrCreate(String cluster, @Nullable String edsS } } // In the case of ref.get() == null, must call cleanQueue() prior to creating a new map entry, - // otherwise the new entry will be deleted be any cleanQueue(). + // otherwise the new entry will be deleted by any later cleanQueue(). cleanQueue(); if (counter != null) { return counter; From 674500d5481059335fe2cdc71d10bc395d368035 Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Wed, 1 Sep 2021 16:49:25 -0700 Subject: [PATCH 5/6] check ref before remove --- .../io/grpc/xds/SharedCallCounterMap.java | 28 ++++++++----------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java index 3011e5ec913..8f720d55094 100644 --- a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java +++ b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java @@ -52,28 +52,19 @@ static SharedCallCounterMap getInstance() { @Override public synchronized AtomicLong getOrCreate(String cluster, @Nullable String edsServiceName) { - AtomicLong counter = null; Map clusterCounters = counters.get(cluster); - if (clusterCounters != null) { - CounterReference ref = clusterCounters.get(edsServiceName); - if (ref != null) { - counter = ref.get(); - } - } - // In the case of ref.get() == null, must call cleanQueue() prior to creating a new map entry, - // otherwise the new entry will be deleted by any later cleanQueue(). - cleanQueue(); - if (counter != null) { - return counter; - } - clusterCounters = counters.get(cluster); if (clusterCounters == null) { clusterCounters = new HashMap<>(); counters.put(cluster, clusterCounters); } - counter = new AtomicLong(); - CounterReference ref = new CounterReference(counter, refQueue, cluster, edsServiceName); - clusterCounters.put(edsServiceName, ref); + CounterReference ref = clusterCounters.get(edsServiceName); + AtomicLong counter; + if (ref == null || (counter = ref.get()) == null) { + counter = new AtomicLong(); + ref = new CounterReference(counter, refQueue, cluster, edsServiceName); + clusterCounters.put(edsServiceName, ref); + } + cleanQueue(); return counter; } @@ -82,6 +73,9 @@ void cleanQueue() { CounterReference ref; while ((ref = (CounterReference) refQueue.poll()) != null) { Map clusterCounter = counters.get(ref.cluster); + if (clusterCounter.get(ref.edsServiceName) != ref) { + continue; + } clusterCounter.remove(ref.edsServiceName); if (clusterCounter.isEmpty()) { counters.remove(ref.cluster); From 01aa6beca50d11b4d6e580f916576efc14b8d23d Mon Sep 17 00:00:00 2001 From: "Penn (Dapeng) Zhang" Date: Thu, 2 Sep 2021 09:40:07 -0700 Subject: [PATCH 6/6] manually call ref.enqueue() --- .../main/java/io/grpc/xds/SharedCallCounterMap.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java index 8f720d55094..7aa55c27429 100644 --- a/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java +++ b/xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java @@ -58,8 +58,14 @@ public synchronized AtomicLong getOrCreate(String cluster, @Nullable String edsS counters.put(cluster, clusterCounters); } CounterReference ref = clusterCounters.get(edsServiceName); - AtomicLong counter; - if (ref == null || (counter = ref.get()) == null) { + AtomicLong counter = null; + if (ref != null) { + counter = ref.get(); + if (counter == null) { + ref.enqueue(); + } + } + if (counter == null) { counter = new AtomicLong(); ref = new CounterReference(counter, refQueue, cluster, edsServiceName); clusterCounters.put(edsServiceName, ref);