fix for AvoidFutureJoinWithoutTimeout false positives #307

jborgers · May 3, 2024 · b001b32 · b001b32
1 parent 475799a
commit b001b32
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 12 deletions.
diff --git a/rulesets/java/jpinpoint-rules.xml b/rulesets/java/jpinpoint-rules.xml
@@ -3026,14 +3026,16 @@ class Foo {
                     <![CDATA[
 (: future.join method reference without timeout in method :)
 //PrimaryExpression/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')
-and not(ancestor::MethodDeclaration//PrimaryPrefix/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')][../../PrimarySuffix//ArgumentList])
+and not(ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimaryPrefix/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')][../../PrimarySuffix//ArgumentList])
+and not (ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimarySuffix[ends-with(@Image, 'Timeout')])
 ]
 /../PrimarySuffix//MethodReference[@Image='join']
 ,
 (: future.join in lambda without timeout in method :)
 //PrimaryExpression/PrimaryPrefix/LambdaExpression//Name[ends-with(@Image, '.join')
-and not(ancestor::MethodDeclaration//PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')]
-[../../PrimarySuffix//ArgumentList])
+and not(ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]
+/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')][../../PrimarySuffix//ArgumentList])
+and not (ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimarySuffix[ends-with(@Image, 'Timeout')])
 ]
 (: join has no arguments, #198 :)
 [../../PrimarySuffix/Arguments[@Size=0]]
@@ -3042,7 +3044,8 @@ and not(ancestor::MethodDeclaration//PrimaryPrefix[pmd-java:typeIs('java.util.co
 //PrimaryExpression/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/../PrimarySuffix[@Image='join'
 (: not bad if has a future.get with (timeout) arguments before the future.join, #300 :)
 and not(ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimaryExpression[PrimarySuffix/Arguments/ArgumentList]
-/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')])]
+/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')])
+and not (../PrimarySuffix[ends-with(@Image, 'Timeout')])]
 (: join has no arguments, #198 :)
 [../PrimarySuffix/Arguments[@Size=0]]
 ]]>

diff --git a/src/main/resources/category/java/concurrent.xml b/src/main/resources/category/java/concurrent.xml
@@ -825,14 +825,16 @@ public class Foo {
                     <![CDATA[
 (: future.join method reference without timeout in method :)
 //PrimaryExpression/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')
-and not(ancestor::MethodDeclaration//PrimaryPrefix/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')][../../PrimarySuffix//ArgumentList])
+and not(ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimaryPrefix/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')][../../PrimarySuffix//ArgumentList])
+and not (ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimarySuffix[ends-with(@Image, 'Timeout')])
 ]
 /../PrimarySuffix//MethodReference[@Image='join']
 ,
 (: future.join in lambda without timeout in method :)
 //PrimaryExpression/PrimaryPrefix/LambdaExpression//Name[ends-with(@Image, '.join')
-and not(ancestor::MethodDeclaration//PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')]
-[../../PrimarySuffix//ArgumentList])
+and not(ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]
+/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')][../../PrimarySuffix//ArgumentList])
+and not (ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimarySuffix[ends-with(@Image, 'Timeout')])
 ]
 (: join has no arguments, #198 :)
 [../../PrimarySuffix/Arguments[@Size=0]]
@@ -841,7 +843,8 @@ and not(ancestor::MethodDeclaration//PrimaryPrefix[pmd-java:typeIs('java.util.co
 //PrimaryExpression/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/../PrimarySuffix[@Image='join'
 (: not bad if has a future.get with (timeout) arguments before the future.join, #300 :)
 and not(ancestor::BlockStatement/preceding-sibling::BlockStatement//PrimaryExpression[PrimarySuffix/Arguments/ArgumentList]
-/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')])]
+/PrimaryPrefix[pmd-java:typeIs('java.util.concurrent.Future')]/Name[ends-with(@Image, '.get') or ends-with(@Image, 'Timeout')])
+and not (../PrimarySuffix[ends-with(@Image, 'Timeout')])]
 (: join has no arguments, #198 :)
 [../PrimarySuffix/Arguments[@Size=0]]
 ]]>

diff --git a/...ces/com/jpinpoint/perf/lang/java/ruleset/concurrent/xml/AvoidFutureJoinWithoutTimeout.xml b/...ces/com/jpinpoint/perf/lang/java/ruleset/concurrent/xml/AvoidFutureJoinWithoutTimeout.xml
@@ -154,15 +154,15 @@ class Issue300 {
 
         CompletableFuture.allOf(firstCompletableFuture, secondCompletableFuture).join(); // bad 8
 
-        Optional<String> userProfile = firstCompletableFuture.get(COMPLETABLE_FUTURE_TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
-        Optional<String> userAccounts = secondCompletableFuture.get(COMPLETABLE_FUTURE_TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
+        Optional<String> userProfile = firstCompletableFuture.get(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
+        Optional<String> userAccounts = secondCompletableFuture.get(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
     }
     public void getUserDetail2Ok(String userId) {
         CompletableFuture<Optional<String>> firstCompletableFuture = firstService.getFirst(userId);
         CompletableFuture<Optional<String>> secondCompletableFuture = secondService.getSecond(userId);
 
-        Optional<String> userProfile = firstCompletableFuture.get(COMPLETABLE_FUTURE_TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
-        Optional<String> userAccounts = secondCompletableFuture.get(COMPLETABLE_FUTURE_TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
+        Optional<String> userProfile = firstCompletableFuture.get(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
+        Optional<String> userAccounts = secondCompletableFuture.get(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
 
        CompletableFuture.allOf(firstCompletableFuture, secondCompletableFuture).join(); //  ok
     }
@@ -176,8 +176,57 @@ class Issue300 {
        CompletableFuture.allOf(firstCompletableFuture, secondCompletableFuture).join(); //  bad 29
     }
 }
+]]></code>
+    </test-code>
+
+    <test-code>
+        <description>Avoid Future Join Without Timeout, chained orTimout before join #307</description>
+        <expected-problems>1</expected-problems>
+        <expected-linenumbers>21</expected-linenumbers>
+        <code><![CDATA[
+import java.util.*;
+import java.util.concurrent.*;
+class Issue3XX {
+    public void getUserDetail4_OK(String userId) {
+        CompletableFuture<Optional<String>> firstCompletableFuture = firstService.getFirst(userId);
+        CompletableFuture<Optional<String>> secondCompletableFuture = secondService.getSecond(userId);
+
+        CompletableFuture.allOf(firstCompletableFuture, secondCompletableFuture)
+                .orTimeout(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS)
+                .join(); // good, was FALSE POSITIVE
+    }
+
+    private Orders getOrders_Bad(OrderSearchOptions searchOptions, List<IGetOrdersService> getOrdersServices) {
+        List<CompletableFuture<Object>> completableFutures = getOrdersServices.stream()
+                .map(orderManager -> {
+                    return CompletableFuture.supplyAsync(() -> i, executor);
+                })
+                .collect(Collectors.toList());
+        List<Object> orderSearchResults = completableFutures.stream()
+                .filter(future -> future.isDone() && !future.isCompletedExceptionally()) // keep only the ones completed -- added to deal with timeout
+                .map(CompletableFuture::join) // bad since NO timeout used above in method 21
+                //.map(future -> future.join())
+                .collect(Collectors.toList());
+        return new Orders(orderSearchResults);
+    }
 
+    private Orders getOrders_OK(OrderSearchOptions searchOptions, List<IGetOrdersService> getOrdersServices) {
 
+        List<CompletableFuture<Object>> completableFutures = getOrdersServices.stream()
+                .map(orderManager -> {
+                    return CompletableFuture.supplyAsync(() -> i, executor).orTimeout(TIMEOUT_IN_MS, TimeUnit.MILLISECONDS);
+                })
+                .collect(Collectors.toList());
+
+        List<Object> orderSearchResults = completableFutures.stream()
+                .filter(future -> future.isDone() && !future.isCompletedExceptionally()) // keep only the ones completed -- added to deal with timeout
+                .map(CompletableFuture::join) // good since timeout used above in method, was FALSE POSITIVE
+                //.map(future -> future.join()) // alternative
+                .collect(Collectors.toList());
+
+        return new Orders(orderSearchResults);
+    }
+}
 ]]></code>
     </test-code>