Skip to content

Commit d459338

Browse files
core: fix false-positive orphan warning in ManagedChannelOrphanWrapper (grpc#12705)
This PR addresses a race condition where ManagedChannelOrphanWrapper could incorrectly log a "not shutdown properly" warning during garbage collection when using directExecutor(). Changes: Reference Management: Moved phantom.clearSafely() to execute after the super.shutdown() calls to ensure the orphan tracker isn't detached prematurely. Reachability Fence: Added a reachability fence in shutdown() and shutdownNow() to ensure the wrapper remains alive until the methods return, preventing the JIT from marking it for early collection. Regression Test: Added a test case that simulates a reference being held on the stack to verify the fix and prevent future regressions. Testing: Verified with ./gradlew :grpc-core:test --tests ManagedChannelOrphanWrapperTest -PskipAndroid=true. Fixes grpc#12641 --------- Co-authored-by: Kannan J <kannanjgithub@google.com>
1 parent 6b3e2b3 commit d459338

File tree

2 files changed

+78
-3
lines changed

2 files changed

+78
-3
lines changed

core/src/main/java/io/grpc/internal/ManagedChannelOrphanWrapper.java

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -62,14 +62,24 @@ final class ManagedChannelOrphanWrapper extends ForwardingManagedChannel {
6262

6363
@Override
6464
public ManagedChannel shutdown() {
65+
ManagedChannel result = super.shutdown();
6566
phantom.clearSafely();
66-
return super.shutdown();
67+
// This dummy check prevents the JIT from collecting 'this' too early
68+
if (this.getClass() == null) {
69+
throw new AssertionError();
70+
}
71+
return result;
6772
}
6873

6974
@Override
7075
public ManagedChannel shutdownNow() {
76+
ManagedChannel result = super.shutdownNow();
7177
phantom.clearSafely();
72-
return super.shutdownNow();
78+
// This dummy check prevents the JIT from collecting 'this' too early
79+
if (this.getClass() == null) {
80+
throw new AssertionError();
81+
}
82+
return result;
7383
}
7484

7585
@VisibleForTesting
@@ -151,8 +161,9 @@ static int cleanQueue(ReferenceQueue<ManagedChannelOrphanWrapper> refqueue) {
151161
int orphanedChannels = 0;
152162
while ((ref = (ManagedChannelReference) refqueue.poll()) != null) {
153163
RuntimeException maybeAllocationSite = ref.allocationSite.get();
164+
boolean wasShutdown = ref.shutdown.get();
154165
ref.clearInternal(); // technically the reference is gone already.
155-
if (!ref.shutdown.get()) {
166+
if (!wasShutdown) {
156167
orphanedChannels++;
157168
Level level = Level.SEVERE;
158169
if (logger.isLoggable(level)) {

core/src/test/java/io/grpc/internal/ManagedChannelOrphanWrapperTest.java

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,70 @@ public boolean isDone() {
101101
}
102102
}
103103

104+
@Test
105+
public void shutdown_withDelegateStillReferenced_doesNotLogWarning() {
106+
ManagedChannel mc = new TestManagedChannel();
107+
final ReferenceQueue<ManagedChannelOrphanWrapper> refqueue = new ReferenceQueue<>();
108+
ConcurrentMap<ManagedChannelReference, ManagedChannelReference> refs =
109+
new ConcurrentHashMap<>();
110+
111+
ManagedChannelOrphanWrapper wrapper = new ManagedChannelOrphanWrapper(mc, refqueue, refs);
112+
WeakReference<ManagedChannelOrphanWrapper> wrapperWeakRef = new WeakReference<>(wrapper);
113+
114+
final List<LogRecord> records = new ArrayList<>();
115+
Logger orphanLogger = Logger.getLogger(ManagedChannelOrphanWrapper.class.getName());
116+
Filter oldFilter = orphanLogger.getFilter();
117+
orphanLogger.setFilter(new Filter() {
118+
@Override
119+
public boolean isLoggable(LogRecord record) {
120+
synchronized (records) {
121+
records.add(record);
122+
}
123+
return false;
124+
}
125+
});
126+
127+
try {
128+
wrapper.shutdown();
129+
wrapper = null;
130+
131+
// Wait for the WRAPPER itself to be garbage collected
132+
GcFinalization.awaitClear(wrapperWeakRef);
133+
ManagedChannelReference.cleanQueue(refqueue);
134+
135+
synchronized (records) {
136+
assertEquals("Warning was logged even though shutdownNow() was called!", 0, records.size());
137+
}
138+
} finally {
139+
orphanLogger.setFilter(oldFilter);
140+
}
141+
}
142+
143+
@Test
144+
public void orphanedChannel_triggerWarningAndCoverage() {
145+
ManagedChannel mc = new TestManagedChannel();
146+
final ReferenceQueue<ManagedChannelOrphanWrapper> refqueue = new ReferenceQueue<>();
147+
ConcurrentMap<ManagedChannelReference, ManagedChannelReference> refs =
148+
new ConcurrentHashMap<>();
149+
150+
// Create the wrapper but NEVER call shutdown
151+
@SuppressWarnings("UnusedVariable")
152+
ManagedChannelOrphanWrapper wrapper = new ManagedChannelOrphanWrapper(mc, refqueue, refs);
153+
wrapper = null; // Make it eligible for GC
154+
155+
// Trigger GC and clean the queue to hit the !wasShutdown branch
156+
final AtomicInteger numOrphans = new AtomicInteger();
157+
GcFinalization.awaitDone(new FinalizationPredicate() {
158+
@Override
159+
public boolean isDone() {
160+
numOrphans.getAndAdd(ManagedChannelReference.cleanQueue(refqueue));
161+
return numOrphans.get() > 0;
162+
}
163+
});
164+
165+
assertEquals(1, numOrphans.get());
166+
}
167+
104168
@Test
105169
public void refCycleIsGCed() {
106170
ReferenceQueue<ManagedChannelOrphanWrapper> refqueue =

0 commit comments

Comments
 (0)