diff --git a/README.md b/README.md index 15daad3c8..472b43df5 100644 --- a/README.md +++ b/README.md @@ -160,6 +160,7 @@ Safe Logging can be found at [github.com/palantir/safe-logging](https://github.c - `GradleCacheableTaskAction`: Gradle plugins should not call `Task.doFirst` or `Task.doLast` with a lambda, as that is not cacheable. See [gradle/gradle#5510](https://github.com/gradle/gradle/issues/5510) for more details. - `PreferBuiltInConcurrentKeySet`: Discourage relying on Guava's `com.google.common.collect.Sets.newConcurrentHashSet()`, when Java's `java.util.concurrent.ConcurrentHashMap.newKeySet()` serves the same purpose. - `JUnit5RuleUsage`: Prevent accidental usage of `org.junit.Rule`/`org.junit.ClassRule` within Junit5 tests +- `DangerousCompletableFutureUsage`: Disallow CompletableFuture asynchronous operations without an Executor. ## com.palantir.baseline-checkstyle Checkstyle rules can be suppressed on a per-line or per-block basis. (It is good practice to first consider formatting diff --git a/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCompletableFutureUsage.java b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCompletableFutureUsage.java new file mode 100644 index 000000000..15c5b02ca --- /dev/null +++ b/baseline-error-prone/src/main/java/com/palantir/baseline/errorprone/DangerousCompletableFutureUsage.java @@ -0,0 +1,103 @@ +/* + * (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.errorprone; + +import com.google.auto.service.AutoService; +import com.google.errorprone.BugPattern; +import com.google.errorprone.VisitorState; +import com.google.errorprone.bugpatterns.BugChecker; +import com.google.errorprone.matchers.Description; +import com.google.errorprone.matchers.Matcher; +import com.google.errorprone.matchers.Matchers; +import com.google.errorprone.matchers.method.MethodMatchers; +import com.google.errorprone.util.ASTHelpers; +import com.sun.source.tree.ExpressionTree; +import com.sun.source.tree.MethodInvocationTree; +import com.sun.tools.javac.code.Symbol; +import com.sun.tools.javac.code.Type; +import java.util.List; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.CompletionStage; +import java.util.concurrent.Executor; +import java.util.function.Supplier; +import java.util.regex.Pattern; + +@AutoService(BugChecker.class) +@BugPattern( + name = "DangerousCompletableFutureUsage", + link = "https://github.com/palantir/gradle-baseline#baseline-error-prone-checks", + linkType = BugPattern.LinkType.CUSTOM, + severity = BugPattern.SeverityLevel.ERROR, + summary = "Disallow CompletableFuture asynchronous operations without an Executor.") +public final class DangerousCompletableFutureUsage + extends BugChecker implements BugChecker.MethodInvocationTreeMatcher { + private static final long serialVersionUID = 1L; + + private static final String ERROR_MESSAGE = "Should not use CompletableFuture methods without specifying a " + + "custom executor service. Doing so has very subtle and potentially severe performance implications, so " + + "in you're better off using your own executor service which allows you to provide instrumentation " + + "and specify the desired parallelism (i.e. the max number of concurrent tasks that will be submitted).\n" + + "By default, CompletableFuture uses the globally shared ForkJoinPool. Fork/join pools implement " + + "work-stealing, where any thread might steal a task from a different thread's queue when blocked " + + "waiting for a subtask to complete. This might not seem like an issue at first glance, but if you rely" + + "on the ForkJoinPool for short tasks extensively throughout your codebase and later on you add one " + + "piece of code that uses the pool for long (e.g. I/O) tasks, the other parts of your codebase that" + + "you'd expect to have consistent performance might experience performance degradation for no apparent " + + "reason.\n" + + "If you're absolutely certain that the ForkJoinPool is correct, please pass ForkJoinPool.commonPool()" + + "directly, ideally with a comment explaining why it is ideal."; + + private static final Matcher SUPPLY_ASYNC = MethodMatchers.staticMethod() + .onClass(CompletableFuture.class.getName()) + .named("supplyAsync") + .withParameters(Supplier.class.getName()); + + private static final Matcher RUN_ASYNC = MethodMatchers.staticMethod() + .onClass(CompletableFuture.class.getName()) + .named("runAsync") + .withParameters(Runnable.class.getName()); + + private static final Matcher STATIC_ASYNC_FACTORY_MATCHERS = + Matchers.anyOf(SUPPLY_ASYNC, RUN_ASYNC); + + private static final Matcher COMPLETION_STAGE_ASYNC_INVOCATION = MethodMatchers.instanceMethod() + .onDescendantOf(CompletionStage.class.getName()) + .withNameMatching(Pattern.compile(".*Async")); + + @Override + public Description matchMethodInvocation(MethodInvocationTree tree, VisitorState state) { + if (STATIC_ASYNC_FACTORY_MATCHERS.matches(tree, state) + || isCompletionStageAsyncMethodWithoutExecutor(tree, state)) { + return buildDescription(tree).setMessage(ERROR_MESSAGE).build(); + } + return Description.NO_MATCH; + } + + private static boolean isCompletionStageAsyncMethodWithoutExecutor(MethodInvocationTree tree, VisitorState state) { + if (!COMPLETION_STAGE_ASYNC_INVOCATION.matches(tree, state)) { + return false; + } + Symbol.MethodSymbol symbol = ASTHelpers.getSymbol(tree); + if (symbol == null) { + // Errors in the AST, allow the build to continue and provide a more helpful compile error. + return false; + } + List parameterTypes = symbol.type.getParameterTypes(); + Type lastParameterType = parameterTypes.get(parameterTypes.size() - 1); + return !ASTHelpers.isSameType(lastParameterType, state.getTypeFromString(Executor.class.getName()), state); + } +} diff --git a/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCompletableFutureUsageTest.java b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCompletableFutureUsageTest.java new file mode 100644 index 000000000..f8c26f12f --- /dev/null +++ b/baseline-error-prone/src/test/java/com/palantir/baseline/errorprone/DangerousCompletableFutureUsageTest.java @@ -0,0 +1,146 @@ +/* + * (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.errorprone; + +import com.google.errorprone.CompilationTestHelper; +import org.junit.Before; +import org.junit.Test; + +public final class DangerousCompletableFutureUsageTest { + private CompilationTestHelper compilationHelper; + + @Before + public void before() { + compilationHelper = CompilationTestHelper.newInstance(DangerousCompletableFutureUsage.class, getClass()); + } + + @Test + public void should_fail_without_executor_supplyAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "class Test {", + " public static final void main(String[] args) {", + " // BUG: Diagnostic contains: Should not use CompletableFuture methods without specifying", + " CompletableFuture.supplyAsync(() -> 1L);", + " }", + "}" + ).doTest(); + } + + @Test + public void should_pass_with_executor_supplyAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "import java.util.concurrent.Executors;", + "class Test {", + " public static final void main(String[] args) {", + " CompletableFuture.supplyAsync(() -> 1L, Executors.newCachedThreadPool());", + " }", + "}" + ).doTest(); + } + + @Test + public void should_fail_without_executor_runAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "class Test {", + " public static final void main(String[] args) {", + " // BUG: Diagnostic contains: Should not use CompletableFuture methods without specifying", + " CompletableFuture.runAsync(() -> {});", + " }", + "}" + ).doTest(); + } + + @Test + public void should_pass_with_executor_runAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "import java.util.concurrent.Executors;", + "class Test {", + " public static final void main(String[] args) {", + " CompletableFuture.runAsync(() -> {}, Executors.newCachedThreadPool());", + " }", + "}" + ).doTest(); + } + + @Test + public void should_fail_without_executor_thenApplyAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "class Test {", + " public static final void main(String[] args) {", + " CompletableFuture future = CompletableFuture.completedFuture(1);", + " // BUG: Diagnostic contains: Should not use CompletableFuture methods without specifying", + " future.thenApplyAsync(i -> i);", + " }", + "}" + ).doTest(); + } + + @Test + public void should_pass_with_executor_thenApplyAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "import java.util.concurrent.Executors;", + "class Test {", + " public static final void main(String[] args) {", + " CompletableFuture.completedFuture(1).thenApplyAsync(i -> i, Executors.newCachedThreadPool());", + " }", + "}" + ).doTest(); + } + + @Test + public void should_fail_without_executor_applyToEitherAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "class Test {", + " public static final void main(String[] args) {", + " CompletableFuture futureOne = CompletableFuture.completedFuture(1);", + " CompletableFuture futureTwo = CompletableFuture.completedFuture(2);", + " // BUG: Diagnostic contains: Should not use CompletableFuture methods without specifying", + " futureOne.applyToEitherAsync(futureTwo, i -> i);", + " }", + "}" + ).doTest(); + } + + @Test + public void should_pass_with_executor_applyToEitherAsync() { + compilationHelper.addSourceLines( + "Test.java", + "import java.util.concurrent.CompletableFuture;", + "import java.util.concurrent.Executors;", + "class Test {", + " public static final void main(String[] args) {", + " CompletableFuture futureOne = CompletableFuture.completedFuture(1);", + " CompletableFuture futureTwo = CompletableFuture.completedFuture(2);", + " futureOne.applyToEitherAsync(futureTwo, i -> i, Executors.newCachedThreadPool());", + " }", + "}" + ).doTest(); + } +} diff --git a/changelog/@unreleased/pr-740.v2.yml b/changelog/@unreleased/pr-740.v2.yml new file mode 100644 index 000000000..558c97703 --- /dev/null +++ b/changelog/@unreleased/pr-740.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Implement DangerousCompletableFutureUsage errorprone check + links: + - https://github.com/palantir/gradle-baseline/pull/740