Repository: netty/netty
Issue: Allow attaching additional metadata to ResourceLeakTrackers

It would be useful to add an API to `ResourceLeakDetector` that allows subclasses to supply custom metadata on `DefaultResourceLeak` creation. This would make it easier to associate detected leaks with parameters of the code that caused the leak, e.g. the test case involved (or in my case, the fuzzer input). 

Right now there is no way to hook into the `ResourceLeakTracker` creation at all, so the only workaround seems to be a custom allocator that `touch`es any buffer on creation, but that doesn't work with separately tracked objects.

IMO a sensible API to implement that would be to add a `protected Object initialHint() { return null; }` method that is called for the "creation" `TraceRecord`. This hint would then be reported alongside the "Created at:" stack trace.

Another alternative is to make `track(T obj)` not be final, so that subclasses can `touch` the record with their own information.

I can make a PR for this, if you think this is a sensible thing to support.

### Netty version

4.1.73

### JVM version (e.g. `java -version`)

openjdk version "17.0.1" 2021-10-19
OpenJDK Runtime Environment Temurin-17.0.1+12 (build 17.0.1+12)
OpenJDK 64-Bit Server VM Temurin-17.0.1+12 (build 17.0.1+12, mixed mode, sharing)

### OS version (e.g. `uname -a`)

Linux yawkat-oracle 5.13.0-28-generic #31-Ubuntu SMP Thu Jan 13 17:41:06 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Issue comments:
@chrisvest WDYT ?
@yawkat @normanmaurer In this PR, we would just be adding this protected do-nothing method, and call it in the right places, right? And then expect down-stream clients to use e.g. `ResourceLeakDetectorFactory` to install custom detectors that override this method? If that's the case, I think it sounds fine, and would be happy to review a PR.

Pull request review comments:
You don't need to allocate a single huge array. You can instead allocate a large volume of smaller arrays, until the `GarbageCollectorMXBean`s tell you a GC cycle has occurred.
Ok I've just tried this but can't get this to work reliably either.

Relying on `GarbageCollectorMXBean` fails for two reasons: "scavenge" GC (young GC I assume?) has its own MX Bean, but might not collect all references. So that bean might show a young GC has happened, but references wouldn't be processed yet, because they're only done in old GC. I've also tried excluding beans with the name "scavenge", which made it more reliable, but still sometimes failed. I think this might be because references can be processed concurrently: https://developers.redhat.com/articles/2021/05/20/shenandoah-garbage-collection-openjdk-16-concurrent-reference-processing – not sure if this applies to non-shenandoah, but it looks like GC can sometimes do reference processing after the GC has already nominally completed, so we can't rely on the MX bean to tell us when references have been processed. It's also "fixed" by adding a `Thread.sleep(10)` just after the enqueue check, which corroborates this.

```java
    private static long getCollectionCount() {
        long count = 0;
        for (GarbageCollectorMXBean bean : garbageCollectorMxBeans) {
            if (bean.getName().equals("scavenge")) {
                continue;
            }
            count += bean.getCollectionCount();
        }
        return count;
    }

    private static void triggerGc() {
        Assumptions.assumeFalse(garbageCollectorMxBeans.isEmpty(), "GarbageCollectorMXBean not available");
        long collectionCountPrevious = getCollectionCount();

        System.gc();

        @SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
        List<Object> storage = new ArrayList<Object>();
        try {
            while (getCollectionCount() == collectionCountPrevious) {
                storage.add(new byte[1024 * 1024]); // 1M
            }
        } catch (OutOfMemoryError ignored) {
        }
        storage.clear();

        try {
            Thread.sleep(10);
        } catch (InterruptedException e) {
        }
    }
```

Another approach I tried is to use a second `ReferenceQueue` with a reference that is definitely unreachable, and simply allocating until that reference has been enqueued. However even this approach was not 100% reliable (maybe ~80%?). I think this might also be due to concurrent reference processing, where there is a race condition between the GC thread enqueuing the reference, and the test thread checking for the reference's presence. The same "fix" of adding a 10ms delay works here too.

```java
    private static void triggerGc() {
        ReferenceQueue<Object> queue = new ReferenceQueue<Object>();
        Object obj = new Object();
        WeakReference<Object> ref = new WeakReference<Object>(obj, queue);
        obj = null;

        System.gc();

        @SuppressWarnings("MismatchedQueryAndUpdateOfCollection")
        List<Object> storage = new ArrayList<Object>();
        try {
            while (queue.poll() == null) {
                storage.add(new byte[1024 * 1024]); // 1M
            }
        } catch (OutOfMemoryError ignored) {
        }
        ref.hashCode();
        storage.clear();

        try {
            TimeUnit.MILLISECONDS.sleep(10);
        } catch (InterruptedException ignored) {
        }
    }
```

Honestly neither solution seems great. The latter is a bit better imo, because it checks that reference processing actually happened, but it's still far from ideal. What do you think?

Patch that resolved the issue:
diff --git a/common/src/main/java/io/netty/util/ResourceLeakDetector.java b/common/src/main/java/io/netty/util/ResourceLeakDetector.java
index be6845683d9..0408e39975a 100644
--- a/common/src/main/java/io/netty/util/ResourceLeakDetector.java
+++ b/common/src/main/java/io/netty/util/ResourceLeakDetector.java
@@ -255,12 +255,12 @@ private DefaultResourceLeak track0(T obj) {
         if (level.ordinal() < Level.PARANOID.ordinal()) {
             if ((PlatformDependent.threadLocalRandom().nextInt(samplingInterval)) == 0) {
                 reportLeak();
-                return new DefaultResourceLeak(obj, refQueue, allLeaks);
+                return new DefaultResourceLeak(obj, refQueue, allLeaks, getInitialHint(resourceType));
             }
             return null;
         }
         reportLeak();
-        return new DefaultResourceLeak(obj, refQueue, allLeaks);
+        return new DefaultResourceLeak(obj, refQueue, allLeaks, getInitialHint(resourceType));
     }
 
     private void clearRefQueue() {
@@ -300,7 +300,7 @@ private void reportLeak() {
                 continue;
             }
 
-            String records = ref.toString();
+            String records = ref.getReportAndClearRecords();
             if (reportedLeaks.add(records)) {
                 if (records.isEmpty()) {
                     reportUntracedLeak(resourceType);
@@ -342,6 +342,15 @@ protected void reportUntracedLeak(String resourceType) {
     protected void reportInstancesLeak(String resourceType) {
     }
 
+    /**
+     * Create a hint object to be attached to an object tracked by this record. Similar to the additional information
+     * supplied to {@link ResourceLeakTracker#record(Object)}, will be printed alongside the stack trace of the
+     * creation of the resource.
+     */
+    protected Object getInitialHint(String resourceType) {
+        return null;
+    }
+
     @SuppressWarnings("deprecation")
     private static final class DefaultResourceLeak<T>
             extends WeakReference<Object> implements ResourceLeakTracker<T>, ResourceLeak {
@@ -367,7 +376,8 @@ private static final class DefaultResourceLeak<T>
         DefaultResourceLeak(
                 Object referent,
                 ReferenceQueue<Object> refQueue,
-                Set<DefaultResourceLeak<?>> allLeaks) {
+                Set<DefaultResourceLeak<?>> allLeaks,
+                Object initialHint) {
             super(referent, refQueue);
 
             assert referent != null;
@@ -378,7 +388,8 @@ private static final class DefaultResourceLeak<T>
             trackedHash = System.identityHashCode(referent);
             allLeaks.add(this);
             // Create a new Record so we always have the creation stacktrace included.
-            headUpdater.set(this, new TraceRecord(TraceRecord.BOTTOM));
+            headUpdater.set(this, initialHint == null ?
+                    new TraceRecord(TraceRecord.BOTTOM) : new TraceRecord(TraceRecord.BOTTOM, initialHint));
             this.allLeaks = allLeaks;
         }
 
@@ -508,7 +519,16 @@ private static void reachabilityFence0(Object ref) {
 
         @Override
         public String toString() {
+            TraceRecord oldHead = headUpdater.get(this);
+            return generateReport(oldHead);
+        }
+
+        String getReportAndClearRecords() {
             TraceRecord oldHead = headUpdater.getAndSet(this, null);
+            return generateReport(oldHead);
+        }
+
+        private String generateReport(TraceRecord oldHead) {
             if (oldHead == null) {
                 // Already closed
                 return EMPTY_STRING;


Test patch:
diff --git a/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java b/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java
index 2a55da2ff78..3947d60f514 100644
--- a/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java
+++ b/common/src/test/java/io/netty/util/ResourceLeakDetectorTest.java
@@ -20,12 +20,18 @@
 
 import java.util.ArrayDeque;
 import java.util.Queue;
+import java.util.UUID;
 import java.util.concurrent.CyclicBarrier;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
+import java.util.concurrent.atomic.AtomicInteger;
 import java.util.concurrent.atomic.AtomicReference;
 
+import static org.assertj.core.api.Assertions.assertThat;
+
 public class ResourceLeakDetectorTest {
+    @SuppressWarnings("unused")
+    private static volatile int sink;
 
     @Test
     @Timeout(value = 60000, unit = TimeUnit.MILLISECONDS)
@@ -98,6 +104,34 @@ private boolean closeResources(boolean checkClosed) {
         assertNoErrors(error);
     }
 
+    @Timeout(10)
+    @Test
+    public void testLeakSetupHints() throws Throwable {
+        DefaultResource.detectorWithSetupHint.initialise();
+        leakResource();
+
+        do {
+            // Trigger GC.
+            System.gc();
+            // Track another resource to trigger refqueue visiting.
+            Resource resource2 = new DefaultResource();
+            DefaultResource.detectorWithSetupHint.track(resource2).close(resource2);
+            // Give the GC something to work on.
+            for (int i = 0; i < 1000; i++) {
+                sink = System.identityHashCode(new byte[10000]);
+            }
+        } while (DefaultResource.detectorWithSetupHint.getLeaksFound() < 1 && !Thread.interrupted());
+
+        assertThat(DefaultResource.detectorWithSetupHint.getLeaksFound()).isOne();
+        DefaultResource.detectorWithSetupHint.assertNoErrors();
+    }
+
+    private static void leakResource() {
+        Resource resource = new DefaultResource();
+        // We'll never close this ResourceLeakTracker.
+        DefaultResource.detectorWithSetupHint.track(resource);
+    }
+
     // Mimic the way how we implement our classes that should help with leak detection
     private static final  class LeakAwareResource implements Resource {
         private final Resource resource;
@@ -123,6 +157,8 @@ private static final class DefaultResource implements Resource {
         // Sample every allocation
         static final TestResourceLeakDetector<Resource> detector = new TestResourceLeakDetector<Resource>(
                 Resource.class, 1, Integer.MAX_VALUE);
+        static final CreationRecordLeakDetector<Resource> detectorWithSetupHint =
+                new CreationRecordLeakDetector<Resource>(Resource.class, 1);
 
         @Override
         public boolean close() {
@@ -172,4 +208,56 @@ void assertNoErrors() throws Throwable {
             ResourceLeakDetectorTest.assertNoErrors(error);
         }
     }
+
+    private static final class CreationRecordLeakDetector<T> extends ResourceLeakDetector<T> {
+        private String canaryString;
+
+        private final AtomicReference<Throwable> error = new AtomicReference<Throwable>();
+        private final AtomicInteger leaksFound = new AtomicInteger(0);
+
+        CreationRecordLeakDetector(Class<?> resourceType, int samplingInterval) {
+            super(resourceType, samplingInterval);
+        }
+
+        public void initialise() {
+            canaryString = "creation-canary-" + UUID.randomUUID();
+            leaksFound.set(0);
+        }
+
+        @Override
+        protected boolean needReport() {
+            return true;
+        }
+
+        @Override
+        protected void reportTracedLeak(String resourceType, String records) {
+            if (!records.contains(canaryString)) {
+                reportError(new AssertionError("Leak records did not contain canary string"));
+            }
+            leaksFound.incrementAndGet();
+        }
+
+        @Override
+        protected void reportUntracedLeak(String resourceType) {
+            reportError(new AssertionError("Got untraced leak w/o canary string"));
+            leaksFound.incrementAndGet();
+        }
+
+        private void reportError(AssertionError cause) {
+            error.compareAndSet(null, cause);
+        }
+
+        @Override
+        protected Object getInitialHint(String resourceType) {
+            return canaryString;
+        }
+
+        int getLeaksFound() {
+            return leaksFound.get();
+        }
+
+        void assertNoErrors() throws Throwable {
+            ResourceLeakDetectorTest.assertNoErrors(error);
+        }
+    }
 }