This is an automated email from the ASF dual-hosted git repository. wu-sheng pushed a commit to branch fix/dsl-classloader-leak-warn-evidence in repository https://gitbox.apache.org/repos/asf/skywalking.git
commit 13df8e5971cd2b3e5f6873819455c29497f586de Author: Wu Sheng <[email protected]> AuthorDate: Thu Jul 2 17:44:34 2026 +0800 Gate the DSL classloader leak WARN on class-unloading GC evidence --- .../oap/server/core/classloader/ClassLoaderGc.java | 174 ++++++++++++++++++++- .../core/classloader/DSLClassLoaderManager.java | 46 ++++-- .../core/classloader/UnloadProbePayload.java | 32 ++++ .../server/core/classloader/ClassLoaderGcTest.java | 93 +++++++++++ 4 files changed, 322 insertions(+), 23 deletions(-) diff --git a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGc.java b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGc.java index f805fd9dd5..dc8427e48c 100644 --- a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGc.java +++ b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGc.java @@ -18,11 +18,16 @@ package org.apache.skywalking.oap.server.core.classloader; +import java.io.ByteArrayOutputStream; +import java.io.IOException; +import java.io.InputStream; import java.lang.ref.PhantomReference; import java.lang.ref.Reference; import java.lang.ref.ReferenceQueue; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.List; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicBoolean; @@ -40,18 +45,43 @@ import lombok.extern.slf4j.Slf4j; * holding the class) will pin the loader and its classes indefinitely. Without observability, * that kind of leak is invisible until heap pressure surfaces hours later. * + * <p><b>Leak evidence, not wall-clock guessing.</b> A classloader that has defined classes is + * only reclaimed by a class-unloading-capable GC cycle (G1 concurrent mark / full GC — young + * collections never unload classes), and an idle heap may not run one for hours. So "retired + * N minutes ago and still uncollected" is NOT a leak signal by itself. To tell a pinned + * loader apart from plain GC inactivity, the graveyard arms an <em>unload probe</em> whenever + * entries are pending: a parent-less throwaway classloader that defines one empty class + * ({@link UnloadProbePayload}) and is immediately dereferenced. The probe has the exact same + * collection requirement as a retired rule loader, so its collection is proof that a + * class-unloading cycle completed after the probe was minted. {@link #leakSuspects} flags an + * entry only when such a cycle ran comfortably after the entry's retirement and the entry + * still survived it. + * * <p>This graveyard is internal to {@link DSLClassLoaderManager}. The manager retires loaders * here via {@code dropRuntime} (full teardown) and {@code retire} (engine-decided "displaced * prior is dead"); a daemon sweeper thread the manager owns drains collected phantoms + - * WARNs on stale entries. No external caller touches this class — every consumer goes - * through the manager. + * WARNs on evidence-backed suspects. No external caller touches this class — every consumer + * goes through the manager. */ @Slf4j final class ClassLoaderGc { + private static final String PROBE_PAYLOAD_CLASS = UnloadProbePayload.class.getName(); + private static final byte[] PROBE_PAYLOAD_BYTECODE = readProbePayloadBytecode(); + private final ReferenceQueue<RuleClassLoader> queue = new ReferenceQueue<>(); private final Map<PhantomReference<RuleClassLoader>, Retired> pending = new ConcurrentHashMap<>(); + private final ReferenceQueue<ClassLoader> probeQueue = new ReferenceQueue<>(); + private final Object probeLock = new Object(); + /** Live probe generation; at most one at a time. Guarded by {@link #probeLock}. The + * phantom ref must stay strongly held here or it would be GC'd before enqueuing. */ + private PhantomReference<ClassLoader> liveProbe; + private long liveProbeMintedAtMs; + /** Latest probe mint-time for which a class-unloading GC cycle is confirmed complete. + * {@code 0} until the first probe collection is observed. */ + private volatile long unloadEvidenceUpToMs; + @Getter private final AtomicLong collectedTotal = new AtomicLong(); @@ -69,39 +99,163 @@ final class ClassLoaderGc { loader.getKind(), loader.getCatalog(), loader.getRule(), loader.getContentHash(), System.currentTimeMillis(), ref); pending.put(ref, retired); + armUnloadProbe(); } /** * Drain collected phantoms from the queue. Returns the entries the JVM confirmed as * unreachable since the last sweep. Entries that remain in {@link #pending()} after this - * call are still suspected leaks. Called by the manager's internal sweeper thread. + * call have not been collected yet — {@link #leakSuspects} decides which of them are + * evidence-backed leaks. Called by the manager's internal sweeper thread. */ Collection<Retired> sweep() { - final java.util.ArrayList<Retired> drained = new java.util.ArrayList<>(); + drainUnloadProbe(); + final List<Retired> drained = new ArrayList<>(); Reference<?> r; while ((r = queue.poll()) != null) { final Retired done = pending.remove(r); if (done != null) { collectedTotal.incrementAndGet(); - log.info("rule loader collected: {}:{}/{} hash={} ttg={}ms", + log.info("rule loader collected: {}:{}/{} hash={} ttg={}ms{}", done.kind() == DSLClassLoaderManager.Kind.BUNDLED ? "bundled" : "runtime-rule", done.catalog().getWireName(), done.rule(), done.contentHashShort(), - System.currentTimeMillis() - done.retiredAtMs()); + System.currentTimeMillis() - done.retiredAtMs(), + done.warnedAlready() + ? " — the earlier leak warning is cleared, the lingering reference has been released" + : ""); drained.add(done); } } + // Re-arm so entries retired after the previous probe was minted get their own + // evidence generation on a later class-unloading cycle. + if (!pending.isEmpty()) { + armUnloadProbe(); + } return drained; } /** * @return snapshot of retired-but-not-yet-GC'd entries. Elevated steadily across many - * sweeps == leak; the manager's sweeper logs WARN per entry older than the - * configured threshold. + * sweeps == leak; the manager's sweeper logs WARN per entry that + * {@link #leakSuspects} confirms against unload evidence. */ Collection<Retired> pending() { return Collections.unmodifiableCollection(pending.values()); } + /** + * Entries whose leak WARN should fire now: a class-unloading GC cycle is confirmed to + * have completed at least {@code settleMs} after the entry's retirement, and the entry + * survived it — so something still strongly references the loader; GC inactivity is + * ruled out. The settle window keeps legitimately transient holders (an apply call + * chain still carrying the displaced bundle on a request thread while the cycle ran) + * from tripping the detector. Each entry is returned at most once across the graveyard's + * lifetime (single-shot {@code warned} latch). + * + * <p>If the probe payload bytecode is unavailable (unreadable resource — effectively + * never), evidence can't be produced; the check degrades to the wall-clock heuristic + * rather than never warning at all. + */ + Collection<Retired> leakSuspects(final long settleMs) { + final List<Retired> out = new ArrayList<>(); + final long evidenceUpToMs = unloadEvidenceUpToMs; + final long nowMs = System.currentTimeMillis(); + for (final Retired r : pending.values()) { + final boolean suspect = PROBE_PAYLOAD_BYTECODE == null + ? nowMs - r.retiredAtMs() > settleMs + : evidenceUpToMs >= r.retiredAtMs() + settleMs; + if (suspect && r.markWarnedIfNotAlready()) { + out.add(r); + } + } + return out; + } + + /** Latest probe mint-time confirmed survived-past by a class-unloading GC cycle; + * {@code 0} while no cycle has been observed. Exposed for the manager's diagnostics + * and for deterministic tests. */ + long unloadEvidenceUpToMs() { + return unloadEvidenceUpToMs; + } + + /** + * Record that a class-unloading GC cycle completed after {@code probeMintedAtMs}. + * Normally driven by {@link #drainUnloadProbe()}; package-private so tests can exercise + * {@link #leakSuspects} without depending on real GC timing. + */ + void recordUnloadEvidence(final long probeMintedAtMs) { + synchronized (probeLock) { + if (probeMintedAtMs > unloadEvidenceUpToMs) { + unloadEvidenceUpToMs = probeMintedAtMs; + } + } + } + + /** Mint a fresh probe if none is live. The probe loader leaves this method with the + * phantom reference as its only remaining reference, so the next class-unloading GC + * cycle is guaranteed to collect it. */ + private void armUnloadProbe() { + if (PROBE_PAYLOAD_BYTECODE == null) { + return; + } + synchronized (probeLock) { + if (liveProbe != null) { + return; + } + final ProbeClassLoader probe = new ProbeClassLoader(); + probe.definePayload(); + liveProbe = new PhantomReference<>(probe, probeQueue); + liveProbeMintedAtMs = System.currentTimeMillis(); + } + } + + private void drainUnloadProbe() { + boolean collected = false; + while (probeQueue.poll() != null) { + collected = true; + } + if (collected) { + synchronized (probeLock) { + recordUnloadEvidence(liveProbeMintedAtMs); + liveProbe = null; + } + } + } + + private static byte[] readProbePayloadBytecode() { + final String resource = "/" + PROBE_PAYLOAD_CLASS.replace('.', '/') + ".class"; + try (InputStream in = ClassLoaderGc.class.getResourceAsStream(resource)) { + if (in == null) { + log.warn("unload-probe payload bytecode not found at {}; " + + "loader leak detection degrades to the wall-clock heuristic", resource); + return null; + } + final ByteArrayOutputStream out = new ByteArrayOutputStream(512); + final byte[] buf = new byte[1024]; + int n; + while ((n = in.read(buf)) > 0) { + out.write(buf, 0, n); + } + return out.toByteArray(); + } catch (final IOException e) { + log.warn("failed to read unload-probe payload bytecode; " + + "loader leak detection degrades to the wall-clock heuristic", e); + return null; + } + } + + /** Parent-less loader whose single defined class subjects it to class-unloading + * collection semantics. Never asked to load anything. */ + private static final class ProbeClassLoader extends ClassLoader { + ProbeClassLoader() { + super(null); + } + + void definePayload() { + defineClass(PROBE_PAYLOAD_CLASS, PROBE_PAYLOAD_BYTECODE, 0, PROBE_PAYLOAD_BYTECODE.length); + } + } + /** Informational record surfaced to the sweeper. Identity is immutable; only the * {@code warned} latch can flip, and only once per lifetime of this {@code Retired}. */ static final class Retired { @@ -157,5 +311,9 @@ final class ClassLoaderGc { boolean markWarnedIfNotAlready() { return warned.compareAndSet(false, true); } + + boolean warnedAlready() { + return warned.get(); + } } } diff --git a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/DSLClassLoaderManager.java b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/DSLClassLoaderManager.java index 629fab2f28..660af90a69 100644 --- a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/DSLClassLoaderManager.java +++ b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/DSLClassLoaderManager.java @@ -75,9 +75,13 @@ public final class DSLClassLoaderManager { * young-gen pause cycle so most retired loaders surface as collected within a couple of * ticks; short enough that a leaked loader's WARN doesn't wait minutes. */ private static final long SWEEP_INTERVAL_SECONDS = 30L; - /** A retired loader still alive past this threshold is WARN'd as a suspected leak. 5 min — - * long enough that a slow GC cycle doesn't cry wolf, short enough that an actual leak is - * surfaced before heap pressure triggers a full GC pause. */ + /** Settle window for the leak WARN: a retired loader is flagged only when a + * class-unloading GC cycle is confirmed to have completed at least this long after the + * retirement and the loader survived it (see {@link ClassLoaderGc#leakSuspects}). + * Wall-clock age alone is never a leak signal — an idle heap may not run a + * class-unloading-capable collection for hours, and warning on age produced false + * alarms after every hot update on quiet OAPs. 5 min keeps transient holders (an apply + * call chain still carrying the displaced bundle on a request thread) out of the WARN. */ private static final long STALE_LOADER_WARN_THRESHOLD_MS = 5L * 60L * 1000L; private final Map<Key, RuleClassLoader> active = new ConcurrentHashMap<>(); @@ -184,9 +188,10 @@ public final class DSLClassLoaderManager { } /** - * Diagnostic — number of retired loaders the JVM has not yet collected. Steady-state - * elevated reading is the leak signal the sweeper WARNs on, surfaced here so operators - * can graph it independently. + * Diagnostic — number of retired loaders the JVM has not yet collected. A reading that + * stays elevated across class-unloading GC cycles is the leak signal (the sweeper WARNs + * on exactly that evidence); an elevated reading on an idle heap merely means no such + * cycle has run yet. Surfaced so operators can graph it independently. */ public int pendingCount() { return graveyard.pending().size(); @@ -233,15 +238,26 @@ public final class DSLClassLoaderManager { log.debug("dsl-classloader-gc: {} loader(s) confirmed collected", collected.size()); } final long nowMs = System.currentTimeMillis(); - for (final ClassLoaderGc.Retired r : graveyard.pending()) { - final long ageMs = nowMs - r.retiredAtMs(); - if (ageMs > STALE_LOADER_WARN_THRESHOLD_MS && r.markWarnedIfNotAlready()) { - log.warn("rule loader leak suspected: {}:{}/{} hash={} pending {} ms " - + "(threshold {}). Check for lingering handler registrations or " - + "samples buffered in DataCarrier partitions.", - r.kind() == Kind.BUNDLED ? "bundled" : "runtime-rule", - r.catalog().getWireName(), r.rule(), r.contentHashShort(), ageMs, - STALE_LOADER_WARN_THRESHOLD_MS); + for (final ClassLoaderGc.Retired r : graveyard.leakSuspects(STALE_LOADER_WARN_THRESHOLD_MS)) { + log.warn("rule loader leak: {}:{}/{} hash={} survived a class-unloading GC cycle " + + "and is still strongly referenced {} ms after retirement. This is not GC " + + "lag — take a heap dump and inspect the GC-root path of this " + + "RuleClassLoader. Common holders: lingering handler registrations, " + + "samples buffered in DataCarrier partitions, open DSL debug sessions.", + r.kind() == Kind.BUNDLED ? "bundled" : "runtime-rule", + r.catalog().getWireName(), r.rule(), r.contentHashShort(), + nowMs - r.retiredAtMs()); + } + if (log.isDebugEnabled()) { + for (final ClassLoaderGc.Retired r : graveyard.pending()) { + if (!r.warnedAlready()) { + log.debug("rule loader {}:{}/{} hash={} pending {} ms awaiting a " + + "class-unloading GC cycle — not a leak signal (an idle heap " + + "can defer collection indefinitely)", + r.kind() == Kind.BUNDLED ? "bundled" : "runtime-rule", + r.catalog().getWireName(), r.rule(), r.contentHashShort(), + nowMs - r.retiredAtMs()); + } } } } catch (final Throwable t) { diff --git a/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/UnloadProbePayload.java b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/UnloadProbePayload.java new file mode 100644 index 0000000000..0b10381c1b --- /dev/null +++ b/oap-server/server-core/src/main/java/org/apache/skywalking/oap/server/core/classloader/UnloadProbePayload.java @@ -0,0 +1,32 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.oap.server.core.classloader; + +/** + * Bytecode payload for {@link ClassLoaderGc}'s unload-evidence probe. Never instantiated and + * never used as a class — the probe reads this class's {@code .class} resource and defines a + * copy of it into a throwaway parent-less classloader, which makes that loader collectible + * only by a class-unloading-capable GC cycle (the same collection requirement a retired + * {@link RuleClassLoader} has). Keep it empty: the smaller the bytecode, the cheaper each + * probe generation. + */ +final class UnloadProbePayload { + private UnloadProbePayload() { + } +} diff --git a/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGcTest.java b/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGcTest.java new file mode 100644 index 0000000000..55e3b7b663 --- /dev/null +++ b/oap-server/server-core/src/test/java/org/apache/skywalking/oap/server/core/classloader/ClassLoaderGcTest.java @@ -0,0 +1,93 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.apache.skywalking.oap.server.core.classloader; + +import java.lang.ref.Reference; +import java.util.Collection; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +/** + * Evidence-gating tests for {@link ClassLoaderGc}. A retired loader must never be flagged as + * a leak on wall-clock age alone — only after a class-unloading GC cycle demonstrably ran + * past the settle window and the loader survived it. Each test uses its own {@code + * ClassLoaderGc} instance, so nothing here touches the {@link DSLClassLoaderManager} + * singleton's graveyard. + * + * <p>Deliberately no real-GC test here: asserting that the unload probe is collected by + * {@code System.gc()} depends on collector behavior (G1/ZGC/Serial timing, {@code + * -XX:+DisableExplicitGC}) and would be a flake source in CI. That physical behavior was + * verified manually across collectors when the probe was introduced; these tests pin the + * gating logic with injected evidence instead, which is collector-independent. + */ +class ClassLoaderGcTest { + + private RuleClassLoader newLoader(final String rule) { + return new RuleClassLoader(DSLClassLoaderManager.Kind.RUNTIME, Catalog.LAL, rule, + "hash-" + rule, ClassLoaderGcTest.class.getClassLoader()); + } + + @Test + void noSuspectWithoutUnloadEvidence() { + final ClassLoaderGc gc = new ClassLoaderGc(); + final RuleClassLoader pinned = newLoader("no-evidence"); + gc.retire(pinned); + // Even with a zero settle window and any wall-clock age, no completed class-unloading + // cycle has been observed, so nothing may be flagged — this is the exact false-alarm + // case (idle heap after a hot update). + assertTrue(gc.leakSuspects(0).isEmpty(), + "no class-unloading GC cycle observed — pending must not be treated as a leak"); + // JIT liveness analysis may end an object's reachability before its local variable + // goes out of scope; the fence guarantees the loader stays "pinned" through the + // assertion above regardless of compiler optimization. + Reference.reachabilityFence(pinned); + } + + @Test + void suspectRequiresEvidencePastSettleWindow() { + final ClassLoaderGc gc = new ClassLoaderGc(); + final RuleClassLoader pinned = newLoader("evidence-gated"); + gc.retire(pinned); + + // A cycle that completed right at retirement doesn't qualify: transient holders (the + // apply call chain) may legitimately have pinned the loader through it. + gc.recordUnloadEvidence(System.currentTimeMillis()); + assertTrue(gc.leakSuspects(60_000).isEmpty(), + "evidence must postdate retirement by the full settle window"); + + // A cycle confirmed past retirement + settle flags the entry — exactly once. + gc.recordUnloadEvidence(System.currentTimeMillis() + 61_000); + final Collection<ClassLoaderGc.Retired> suspects = gc.leakSuspects(60_000); + assertEquals(1, suspects.size()); + assertEquals("evidence-gated", suspects.iterator().next().rule()); + assertTrue(gc.leakSuspects(60_000).isEmpty(), "leak WARN latch is one-shot per loader"); + + Reference.reachabilityFence(pinned); + } + + @Test + void recordUnloadEvidenceKeepsMaximum() { + final ClassLoaderGc gc = new ClassLoaderGc(); + gc.recordUnloadEvidence(100L); + gc.recordUnloadEvidence(50L); + assertEquals(100L, gc.unloadEvidenceUpToMs(), "older evidence must not regress the watermark"); + } +}
