Gauges should be registered with registerWithReplacement

README.md

@@ -185,6 +185,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c
 - `AssertjPrimitiveComparison`: Prefer using AssertJ fluent comparisons over logic in an assertThat statement.
 - `ExceptionSpecificity`: Prefer more specific catch types than Exception and Throwable.
 - `ThrowSpecificity`: Prefer to declare more specific `throws` types than Exception and Throwable.
+- `UnsafeGaugeRegistration`: Use TaggedMetricRegistry.registerWithReplacement over TaggedMetricRegistry.gauge.
 
 ### Programmatic Application
 

baseline-error-prone/build.gradle

@@ -20,6 +20,7 @@ dependencies {
     testCompile 'commons-lang:commons-lang'
     testCompile 'org.assertj:assertj-core'
     testCompile 'org.jooq:jooq'
+    testCompile 'com.palantir.tritium:tritium-registry'
     testImplementation 'org.junit.jupiter:junit-jupiter'
     testRuntimeOnly 'org.junit.jupiter:junit-jupiter-migrationsupport'
 

baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java

@@ -0,0 +1,91 @@
+/*
+ * (c) Copyright 2018 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed 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 com.palantir.baseline.errorprone;
+
+import com.google.auto.service.AutoService;
+import com.google.errorprone.BugPattern;
+import com.google.errorprone.VisitorState;
+import com.google.errorprone.bugpatterns.AbstractReturnValueIgnored;
+import com.google.errorprone.bugpatterns.BugChecker;
+import com.google.errorprone.fixes.SuggestedFixes;
+import com.google.errorprone.matchers.Description;
+import com.google.errorprone.matchers.Matcher;
+import com.google.errorprone.matchers.method.MethodMatchers;
+import com.sun.source.tree.ExpressionTree;
+import com.sun.source.tree.MethodInvocationTree;
+import com.sun.tools.javac.code.Symbol;
+
+@AutoService(BugChecker.class)
+@BugPattern(
+        name = "UnsafeGaugeRegistration",
+        link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks",
+        linkType = BugPattern.LinkType.CUSTOM,
+        providesFix = BugPattern.ProvidesFix.REQUIRES_HUMAN_ATTENTION,
+        severity = BugPattern.SeverityLevel.WARNING,
+        summary = "Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and can result in subtle "
+                + "resource leaks. Prefer replacing existing gauge values.\n"
+                // This check may begin to fail after a version upgrade, where fixes aren't automatically applied
+                + "This can be fixed automatically using "
+                + "./gradlew compileJava compileTestJava -PerrorProneApply=UnsafeGaugeRegistration")
+public final class UnsafeGaugeRegistration extends AbstractReturnValueIgnored {
+
+    private static final String TAGGED_REGISTRY = "com.palantir.tritium.metrics.registry.TaggedMetricRegistry";
+    private static final String REGISTER_WITH_REPLACEMENT = "registerWithReplacement";
+    private static final Matcher<ExpressionTree> MATCHER = MethodMatchers.instanceMethod()
+            .onDescendantOf(TAGGED_REGISTRY)
+            .named("gauge")
+            .withParameters("com.palantir.tritium.metrics.registry.MetricName", "com.codahale.metrics.Gauge");
+
+    @Override
+    public Matcher<? super ExpressionTree> specializedMatcher() {
+        return MATCHER;
+    }
+
+    // Override matchMethodInvocation from AbstractReturnValueIgnored to apply our suggested fix.
+    @Override
+    public Description matchMethodInvocation(MethodInvocationTree methodInvocationTree, VisitorState state) {
+        Description description = super.matchMethodInvocation(methodInvocationTree, state);
+        if (Description.NO_MATCH.equals(description)
+                || !hasRegisterWithReplacement(state)
+                || TestCheckUtils.isTestCode(state)) {
+            return Description.NO_MATCH;
+        }
+        return buildDescription(methodInvocationTree)
+                .addFix(SuggestedFixes.renameMethodInvocation(methodInvocationTree, REGISTER_WITH_REPLACEMENT, state))
+                .build();
+    }
+
+    /**
+     * TaggedMetricRegistry.registerWithReplacement was added in Tritium 0.16.1, avoid flagging older versions.
+     */
+    private static boolean hasRegisterWithReplacement(VisitorState state) {
+        Symbol symbol = state.getSymbolFromString(TAGGED_REGISTRY);
+        if (!(symbol instanceof Symbol.ClassSymbol)) {
+            return false;
+        }
+        Symbol.ClassSymbol classSymbol = (Symbol.ClassSymbol) symbol;
+        for (Symbol enclosed : classSymbol.getEnclosedElements()) {
+            if (enclosed instanceof Symbol.MethodSymbol) {
+                Symbol.MethodSymbol enclosedMethod = (Symbol.MethodSymbol) enclosed;
+                if (enclosedMethod.name.contentEquals(REGISTER_WITH_REPLACEMENT)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+}

baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java

@@ -0,0 +1,91 @@
+/*
+ * (c) Copyright 2018 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed 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 com.palantir.baseline.errorprone;
+
+import com.google.errorprone.BugCheckerRefactoringTestHelper;
+import com.google.errorprone.CompilationTestHelper;
+import org.junit.jupiter.api.Test;
+
+class UnsafeGaugeRegistrationTest {
+
+    @Test
+    void testFix() {
+        RefactoringValidator.of(new UnsafeGaugeRegistration(), getClass())
+                .addInputLines(
+                        "Test.java",
+                        "import com.palantir.tritium.metrics.registry.MetricName;",
+                        "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;",
+                        "class Test {",
+                        "   void f(TaggedMetricRegistry registry, MetricName name) {",
+                        "       registry.gauge(name, () -> 1);",
+                        "   }",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.palantir.tritium.metrics.registry.MetricName;",
+                        "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;",
+                        "class Test {",
+                        "   void f(TaggedMetricRegistry registry, MetricName name) {",
+                        "       registry.registerWithReplacement(name, () -> 1);",
+                        "   }",
+                        "}"
+                )
+                .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+    }
+
+    @Test
+    void testKnownBug() {
+        RefactoringValidator.of(new UnsafeGaugeRegistration(), getClass())
+                .addInputLines(
+                        "Test.java",
+                        "import com.codahale.metrics.Gauge;",
+                        "import com.palantir.tritium.metrics.registry.MetricName;",
+                        "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;",
+                        "class Test {",
+                        "   void f(TaggedMetricRegistry registry, MetricName name, Gauge<?> gauge) {",
+                        "       registry.gauge(name, gauge);",
+                        "   }",
+                        "}")
+                .addOutputLines(
+                        "Test.java",
+                        "import com.codahale.metrics.Gauge;",
+                        "import com.palantir.tritium.metrics.registry.MetricName;",
+                        "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;",
+                        "class Test {",
+                        "   void f(TaggedMetricRegistry registry, MetricName name, Gauge<?> gauge) {",
+                        //      This isn't right. Filed https://github.com/google/error-prone/issues/1451
+                        "       registry.gauge(name, registerWithReplacement);",
+                        "   }",
+                        "}"
+                )
+                .doTest(BugCheckerRefactoringTestHelper.TestMode.TEXT_MATCH);
+    }
+
+    @Test
+    void testNegative() {
+        CompilationTestHelper.newInstance(UnsafeGaugeRegistration.class, getClass()).addSourceLines(
+                "Test.java",
+                "import com.codahale.metrics.Gauge;",
+                "import com.palantir.tritium.metrics.registry.MetricName;",
+                "import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;",
+                "class Test {",
+                "   Gauge<?> f(TaggedMetricRegistry registry, MetricName name) {",
+                "       return registry.gauge(name, () -> 1);",
+                "   }",
+                "}").doTest();
+    }
+}

baseline-refaster-rules/build.gradle

@@ -7,9 +7,13 @@ dependencies {
     implementation 'com.google.errorprone:error_prone_refaster'
     implementation 'org.assertj:assertj-core'
     implementation 'org.mockito:mockito-core'
+    implementation 'com.palantir.tritium:tritium-registry'
 
     testCompile 'junit:junit'
     testCompile project(':baseline-refaster-testing')
+    testCompile 'org.immutables:value::annotations'
+
+    compileOnly 'org.immutables:value::annotations'
 }
 
 // Do not apply refaster rules

baseline-refaster-rules/src/main/java/com/palantir/baseline/refaster/TritiumReplaceGauge.java

@@ -0,0 +1,47 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed 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 com.palantir.baseline.refaster;
+
+import com.codahale.metrics.Gauge;
+import com.google.errorprone.refaster.annotation.AfterTemplate;
+import com.google.errorprone.refaster.annotation.BeforeTemplate;
+import com.palantir.tritium.metrics.registry.MetricName;
+import com.palantir.tritium.metrics.registry.TaggedMetricRegistry;
+
+/**
+ * Remove unnecessary 'remove' invocation prior to a 'registerWithReplacement'.
+ * This refaster rule pairs with the 'UnsafeGaugeRegistration' error-prone rule
+ * to replace 'registry.remove(name); registry.gauge(name, value);' with a
+ * single 'registerWithReplacement' call.
+ * This refaster rule intentionally doesn't check the 'gauge' method in order to
+ * avoid creating changes that don't compile when older versions of Tritium are
+ * present.
+ */
+public final class TritiumReplaceGauge<T> {
+
+    @BeforeTemplate
+    void before(TaggedMetricRegistry registry, MetricName name, Gauge<T> gauge) {
+        registry.remove(name);
+        registry.registerWithReplacement(name, gauge);
+    }
+
+    @AfterTemplate
+    void after(TaggedMetricRegistry registry, MetricName name, Gauge<T> gauge) {
+        registry.registerWithReplacement(name, gauge);
+    }
+
+}

baseline-refaster-rules/src/test/java/com/palantir/baseline/refaster/TritiumReplaceGaugeTest.java

@@ -0,0 +1,52 @@
+/*
+ * (c) Copyright 2019 Palantir Technologies Inc. All rights reserved.
+ *
+ * Licensed 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 com.palantir.baseline.refaster;
+
+import org.junit.Test;
+
+public class TritiumReplaceGaugeTest {
+
+    @Test
+    public void test() {
+        RefasterTestHelper
+                .forRefactoring(TritiumReplaceGauge.class)
+                .withInputLines(
+                        "Test",
+                        "import com.codahale.metrics.Gauge;",
+                        "import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;",
+                        "import com.palantir.tritium.metrics.registry.MetricName;",
+                        "public class Test {",
+                        "  void f(DefaultTaggedMetricRegistry registry) {",
+                        "    MetricName name = MetricName.builder().safeName(\"foo\").build();",
+                        "    registry.remove(name);",
+                        "    registry.registerWithReplacement(name, () -> 1);",
+                        "  }",
+                        "}")
+                .hasOutputLines(
+                        "import com.codahale.metrics.Gauge;",
+                        "import com.palantir.tritium.metrics.registry.DefaultTaggedMetricRegistry;",
+                        "import com.palantir.tritium.metrics.registry.MetricName;",
+                        "public class Test {",
+                        "  void f(DefaultTaggedMetricRegistry registry) {",
+                        "    MetricName name = MetricName.builder().safeName(\"foo\").build();",
+                        "    registry.registerWithReplacement(name, () -> 1);",
+                        "    ",
+                        "  }",
+                        "}");
+    }
+
+}

changelog/@unreleased/pr-1123.v2.yml

@@ -0,0 +1,11 @@
+type: improvement
+improvement:
+  description: |-
+    Using TaggedMetricRegistry.gauge is equivalent to map.putIfAbsent, and
+    can result in subtle resource leaks. Prefer replacing existing gauge
+    values using `registerWithReplacement`.
+
+    This check doesn't apply unless a new enough version of Tritium
+    is available on the compilation classpath.
+  links:
+  - https://github.com/palantir/gradle-baseline/pull/1123

gradle-baseline-java/src/main/groovy/com/palantir/baseline/extensions/BaselineErrorProneExtension.java

@@ -48,6 +48,7 @@ public class BaselineErrorProneExtension {
             "ThrowError",
             // TODO(ckozak): re-enable pending scala check
             //"ThrowSpecificity",
+            "UnsafeGaugeRegistration",
 
             // Built-in checks
             "ArrayEquals",