From 72c68e91f55c56dc1bf5e069b9cd377766909ee3 Mon Sep 17 00:00:00 2001
From: brharrington <brharrington@gmail.com>
Date: Thu, 28 Jun 2018 22:16:15 -0700
Subject: [PATCH] avoid mem leak for improper use of registerObject (#451)

Calling `Monitors.registerObject` with a monitor such as
a `BasicCounter` instance will register an empty composite
monitor with an id like `default:class=BasicCounter`. When
using the spectator integration this would result is a
memory leak because it needs to track the instances to be
able to aggregate the results.

This change updates the registration to remove any previous
copies and overwrite which is the default for servo. It also
double checks if it is an empty basic composite and just
ignores those since they will never have any monitors to
collect.

It is a bit difficult to test without a bunch of reflection
to dig into the guts of the meter state. I did reproduce in
a debugger using the following test case and confirm the
state is no longer growing:

```java
@Test
public void testBasicCounterLoop() {
  for (int i = 0; i < 1000; ++i) {
    BasicCounter c = new BasicCounter(CONFIG);
    Monitors.registerObject(c);
    c.increment();
  }
  assertEquals(1000, registry.counter(ID).count());
}
```
---
 .../java/com/netflix/servo/SpectatorContext.java | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/servo-core/src/main/java/com/netflix/servo/SpectatorContext.java b/servo-core/src/main/java/com/netflix/servo/SpectatorContext.java
index 53b6cbdb..9e84aa5e 100644
--- a/servo-core/src/main/java/com/netflix/servo/SpectatorContext.java
+++ b/servo-core/src/main/java/com/netflix/servo/SpectatorContext.java
@@ -15,6 +15,7 @@
  */
 package com.netflix.servo;
 
+import com.netflix.servo.monitor.BasicCompositeMonitor;
 import com.netflix.servo.monitor.CompositeMonitor;
 import com.netflix.servo.monitor.Monitor;
 import com.netflix.servo.monitor.MonitorConfig;
@@ -138,12 +139,23 @@ public static PolledMeter.Builder polledGauge(MonitorConfig config) {
   public static void register(Monitor<?> monitor) {
     if (monitor instanceof SpectatorMonitor) {
       ((SpectatorMonitor) monitor).initializeSpectator(BasicTagList.EMPTY);
-    } else {
-      PolledMeter.monitorMeter(registry, new ServoMeter(monitor));
+    } else if (!isEmptyComposite(monitor)) {
+      ServoMeter m = new ServoMeter(monitor);
+      PolledMeter.remove(registry, m.id());
+      PolledMeter.monitorMeter(registry, m);
       monitorMonitonicValues(monitor);
     }
   }
 
+  /**
+   * A basic composite has an immutable list, if it is empty then will never provide any
+   * useful monitors. This can happen if a monitor type is used with Monitors.registerObject.
+   */
+  private static boolean isEmptyComposite(Monitor<?> monitor) {
+    return (monitor instanceof BasicCompositeMonitor)
+        && ((BasicCompositeMonitor) monitor).getMonitors().isEmpty();
+  }
+
   private static boolean isCounter(MonitorConfig config) {
     return "COUNTER".equals(config.getTags().getValue("type"));
   }