Skip to content

Commit

Permalink
avoid mem leak for improper use of registerObject (#451)
Browse files Browse the repository at this point in the history
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());
}
```
  • Loading branch information
brharrington committed Jun 29, 2018
1 parent 2d27fda commit 72c68e9
Showing 1 changed file with 14 additions and 2 deletions.
16 changes: 14 additions & 2 deletions servo-core/src/main/java/com/netflix/servo/SpectatorContext.java
Expand Up @@ -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;
Expand Down Expand Up @@ -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"));
}
Expand Down

0 comments on commit 72c68e9

Please sign in to comment.