-
Notifications
You must be signed in to change notification settings - Fork 135
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Gauges should be registered with
registerWithReplacement (#1123)
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.
- Loading branch information
1 parent
020ebae
commit af50c26
Showing
16 changed files
with
362 additions
and
17 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
91 changes: 91 additions & 0 deletions
91
...e-error-prone/src/main/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistration.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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.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(MoreSuggestedFixes.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; | ||
| } | ||
| } |
91 changes: 91 additions & 0 deletions
91
...ror-prone/src/test/java/com/palantir/baseline/errorprone/UnsafeGaugeRegistrationTest.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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) {", | ||
| // Tests our workaround for https://github.com/google/error-prone/issues/1451 | ||
| " registry.registerWithReplacement(name, gauge);", | ||
| " }", | ||
| "}" | ||
| ) | ||
| .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(); | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.