Skip to content

xds: Fix WeakReference bug in SharedCallCounterMap #8466

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Sep 2, 2021
Merged

xds: Fix WeakReference bug in SharedCallCounterMap #8466

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
69eb4bc
add regression test
dapengzhang0 Sep 1, 2021
d46dd18
fix bug
dapengzhang0 Sep 1, 2021
50c637f
fix wrong fix
dapengzhang0 Sep 1, 2021
06cb848
fix comment
dapengzhang0 Sep 1, 2021
674500d
check ref before remove
dapengzhang0 Sep 1, 2021
01aa6be
manually call ref.enqueue()
dapengzhang0 Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions xds/src/main/java/io/grpc/xds/SharedCallCounterMap.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -73,6 +79,9 @@ void cleanQueue() {
CounterReference ref;
while ((ref = (CounterReference) refQueue.poll()) != null) {
Map<String, CounterReference> clusterCounter = counters.get(ref.cluster);
Copy link
Contributor Author

@dapengzhang0 dapengzhang0 Sep 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing clusterCounter shouldn't be null, because refs should be enqueued in the same order as the order of underlying referents being nullified by garbage collector. But I did not see javadoc explicitly say that.

Is there any risk of NPE in extreme race case like the following?
ref1.referent nullified by gc => ref2 created and put in the counters map => ref2.referent nullified by gc => ref2 enqueued => ref1 enqueued.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With C1 it doesn't seem too far-fetched, especially if enqueuing is a separate stage of the process from clearing. I doubt it would actually happen, but it seems fair to consider.

A simple solution for that is to call ref.enqueue() if ref.get() == null, before replacing the reference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. TIL thanks.

if (clusterCounter.get(ref.edsServiceName) != ref) {
continue;
}
clusterCounter.remove(ref.edsServiceName);
if (clusterCounter.isEmpty()) {
counters.remove(ref.cluster);
Expand Down
18 changes: 18 additions & 0 deletions xds/src/test/java/io/grpc/xds/SharedCallCounterMapTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, you can call ref.clear() and ref.enqueue() manually instead of relying on GC.

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();
}
}