SONARJAVA-4406 FP on S2142 when the InterruptedException is caught in an inner try-catch

Author: kaufco

java-checks-test-sources/src/main/java/checks/InterruptedExceptionCheck.java

@@ -25,7 +25,7 @@ public void run1 () {
     }catch (java.io.IOException e) {
       LOGGER.log(Level.WARN, "Interrupted!", e);
     }catch (InterruptedException e) { // Noncompliant [[sc=13;ec=35]] {{Either re-interrupt this method or rethrow the "InterruptedException" that can be caught here.}}
-        LOGGER.log(Level.WARN, "Interrupted!", e);
+      LOGGER.log(Level.WARN, "Interrupted!", e);
     }
   }
 
@@ -57,10 +57,10 @@ public void runInterrupted() {
     try {
       throw  new InterruptedException();
     } catch (InterruptedException e) {
-        LOGGER.log(Level.WARN, "Interrupted!", e);
-        // clean up state...
-        Thread.currentThread().interrupt();
-      }
+      LOGGER.log(Level.WARN, "Interrupted!", e);
+      // clean up state...
+      Thread.currentThread().interrupt();
+    }
     try {
       while (true) {
         throw  new InterruptedException();
@@ -71,7 +71,7 @@ public void runInterrupted() {
       // clean up state...
       new Interruptable().interrupt();
     }
-    }
+  }
 
   public Object getNextTask(BlockingQueue<Object> queue) {
     boolean interrupted = false;
@@ -96,14 +96,14 @@ class Interruptable {
   static final Log LOGGER = null;
 
   void interrupt() {
-    
+
   }
 
   private static void waitForNextExecution(Set<Runnable> running, LongSupplier waitTimeoutMillis) {
     try {
       Thread.sleep(waitTimeoutMillis.getAsLong());
     } catch (InterruptedException e) { //Compliant
-      cancelAllSubTasksAndInterrupt(running); 
+      cancelAllSubTasksAndInterrupt(running);
     }
   }
 
@@ -113,28 +113,28 @@ private static void cancelAllSubTasksAndInterrupt(Set<Runnable> subTasks) {
     }
     Thread.currentThread().interrupt();
   }
-  
+
   private static void waitForNextExecution1(Set<Runnable> running, LongSupplier waitTimeoutMillis) {
     try {
       Thread.sleep(waitTimeoutMillis.getAsLong());
     } catch (InterruptedException e) { // Noncompliant, too many levels
-      cancelAllSubTasksAndInterrupt1(running); 
+      cancelAllSubTasksAndInterrupt1(running);
     }
   }
 
   private static void cancelAllSubTasksAndInterrupt1(Set<Runnable> subTasks) {
     cancelAllSubTasksAndInterrupt2(subTasks);
   }
 
-  private static void cancelAllSubTasksAndInterrupt2(Set<Runnable> subTasks) { 
+  private static void cancelAllSubTasksAndInterrupt2(Set<Runnable> subTasks) {
     cancelAllSubTasksAndInterrupt3(subTasks);
   }
 
   private static void cancelAllSubTasksAndInterrupt3(Set<Runnable> subTasks) {
     cancelAllSubTasksAndInterrupt4(subTasks);
     if (LOGGER != null )throw new RuntimeException();
   }
-  
+
   private static void cancelAllSubTasksAndInterrupt4(Set<Runnable> subTasks) {
     for (Runnable task : subTasks) {
       System.out.println("--- waitForNextExecution: Service interrupted. Cancel execution of task {}.");
@@ -275,7 +275,6 @@ public void throwNewInterruptedExceptionFromFunction() throws InterruptedExcepti
   }
 
   public void throwNewCustomizedInterruptedExceptionFromFunction() throws InterruptedException {
-
     try {
       throwsInterruptedException();
     } catch (InterruptedException e) { // Compliant
@@ -302,7 +301,6 @@ public void throwNewCustomizedInterruptedExceptionFromFunction() throws Interrup
   }
 
   public void rethrowSameException() throws InterruptedException {
-
     try {
       throwsInterruptedException();
     } catch (InterruptedException e) { // Compliant
@@ -340,7 +338,6 @@ public void rethrowSameException() throws InterruptedException {
   }
 
   public void cutControlFlow() throws InterruptedException {
-
     try {
       throwsInterruptedException();
     } catch (InterruptedException e) { // Noncompliant, because neither foo nor bar belong to the control flow of this catch block.
@@ -359,11 +356,74 @@ void bar() throws InterruptedException {
     try {
       throwsInterruptedException();
     } catch (InterruptedException e) { // Noncompliant, because neither foo, nor bar belong to the control flow of this catch block.
-        Runnable foo = () -> Thread.currentThread().interrupt();
-        Action bar = () -> {
-          throw new InterruptedException();
-        };
+      Runnable foo = () -> Thread.currentThread().interrupt();
+      Action bar = () -> {
+        throw new InterruptedException();
+      };
+    }
+  }
+
+  void falsePositivesSonarjava4406() {
+    try {
+      try {
+        throwsInterruptedException();
+      } catch (InterruptedException ie) { // Noncompliant, because interruption state is lost.
+      }
+    } catch (Exception e) { // Compliant, because inner try does not throw an InterruptedException
+    }
+
+    try {
+      try {
+        throwsInterruptedException();
+      } catch (InterruptedException ie) { // Compliant
+        Thread.currentThread().interrupt();
+      }
+    } catch (Exception e) { // Compliant, because inner try does not throw an InterruptedException
     }
+
+    try {
+      try {
+        throwsInterruptedException();
+      } catch (InterruptedException ie) { // Compliant
+        throw new InterruptedException();
+      }
+    } catch (InterruptedException e) { // Noncompliant, because explicitly catch InterruptedException
+    }
+
+    try {
+      try {
+        throwsInterruptedException();
+      } catch (RuntimeException ie) { // Compliant
+      }
+    } catch (Exception e) { // Noncompliant, because inner try may throw an InterruptedException
+    }
+
+  }
+
+  public void throwInterruptedExceptionFromResourceList() throws InterruptedException {
+    try {
+      try(AutoCloseable autocloseable = getAutoCloseableThrowsInterruptedException()) {
+        doSomething();
+      } catch (IOException ie) {
+        doSomething();
+      }
+    } catch (Exception e) { // Noncompliant
+    }
+  }
+
+  public void throwInterruptedExceptionWithinThrowStatement() throws InterruptedException {
+    try {
+      throw getRuntimeExceptionThrowsInterruptedException();
+    } catch (Exception e) { // Noncompliant
+    }
+  }
+
+  private AutoCloseable getAutoCloseableThrowsInterruptedException() throws InterruptedException {
+    throw getInterruptedException();
+  }
+
+  private RuntimeException getRuntimeExceptionThrowsInterruptedException() throws InterruptedException {
+    throw getInterruptedException();
   }
 
   @FunctionalInterface
@@ -414,15 +474,11 @@ private static void interruptByThrowingCustomizedInterruptedExceptionL4() throws
     throw new CustomizedInterruptedException();
   }
 
-  public void throwsException() throws Exception {
+  public static void throwsException() throws Exception {
     throw new Exception();
   }
 
-  public void throwsInterruptedException() throws InterruptedException {
-    throw new InterruptedException();
-  }
-
-  public void throwsCustomizedInterruptedException() throws InterruptedException {
+  public static void throwsInterruptedException() throws InterruptedException {
     throw new InterruptedException();
   }
 

java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java

@@ -33,12 +33,12 @@
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
 import org.sonar.plugins.java.api.semantic.MethodMatchers;
-import org.sonar.plugins.java.api.semantic.Symbol;
 import org.sonar.plugins.java.api.semantic.Type;
 import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
 import org.sonar.plugins.java.api.tree.BlockTree;
 import org.sonar.plugins.java.api.tree.CatchTree;
 import org.sonar.plugins.java.api.tree.ClassTree;
+import org.sonar.plugins.java.api.tree.ExpressionTree;
 import org.sonar.plugins.java.api.tree.MethodInvocationTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.ThrowStatementTree;
@@ -84,6 +84,7 @@ public void visitNode(Tree tree) {
     TryStatementTree tryStatementTree = (TryStatementTree) tree;
 
     withinInterruptingFinally.addFirst(isFinallyInterrupting(tryStatementTree.finallyBlock()));
+
     for (CatchTree catchTree : tryStatementTree.catches()) {
       VariableTree catchParameter = catchTree.parameter();
       List<Type> caughtTypes = getCaughtTypes(catchParameter);
@@ -102,14 +103,14 @@ public void visitNode(Tree tree) {
   }
 
   private void reportIfThrowInterruptInBlock(BlockTree blockTree, CatchTree catchTree) {
-    MethodTreeUtils.MethodInvocationCollector collector = new MethodTreeUtils.MethodInvocationCollector(InterruptedExceptionCheck::throwInterruptedException);
+    InterruptingStatementCollector collector = new InterruptingStatementCollector();
     blockTree.accept(collector);
     List<Tree> invocationInterrupting = collector.getInvocationTree();
 
     if (!invocationInterrupting.isEmpty() && wasNotInterrupted(catchTree)) {
       reportIssue(catchTree.parameter(), String.format(MESSAGE, "InterruptedException"),
         invocationInterrupting.stream()
-          .map(t -> new JavaFileScannerContext.Location("Method invocation throwing InterruptedException.", t))
+          .map(t -> new JavaFileScannerContext.Location("Statement throwing InterruptedException.", t))
           .collect(Collectors.toList()),
         null);
     }
@@ -148,9 +149,36 @@ private static boolean isFinallyInterrupting(@Nullable BlockTree blockTree) {
     return blockVisitor.threadInterrupted;
   }
 
-  private static boolean throwInterruptedException(Symbol.MethodSymbol symbol) {
-    return !symbol.isUnknown()
-      && symbol.thrownTypes().stream().anyMatch(INTERRUPTING_TYPE_PREDICATE);
+  private static boolean isInterruptingExceptionExpression(ExpressionTree expressionTree) {
+    return INTERRUPTING_TYPE_PREDICATE.test(expressionTree.symbolType());
+  }
+
+  private static class InterruptingStatementCollector extends MethodTreeUtils.MethodInvocationCollector {
+
+    public InterruptingStatementCollector() {
+      super(
+        symbol -> symbol.thrownTypes().stream().anyMatch(INTERRUPTING_TYPE_PREDICATE)
+      );
+    }
+
+    @Override
+    public void visitTryStatement(TryStatementTree tryStatementTree) {
+      // If inner `try` statement catches interrupting types: cut analysis of its try block, because possible
+      // interruptions thrown there are handled within the scope of the catch block.
+      // Yet, all other blocks (resources, catch, finally) of inner `try`s must still be analyzed, because possible
+      // interruptions thrown there must be handled within the scope of the outer try.
+
+      boolean isCatchingAnyInterruptingTypes = tryStatementTree.catches().stream().anyMatch(catchTree ->
+        getCaughtTypes(catchTree.parameter()).stream().anyMatch(INTERRUPTING_TYPE_PREDICATE)
+      );
+
+      scan(tryStatementTree.resourceList());
+      if (!isCatchingAnyInterruptingTypes) {
+        scan(tryStatementTree.block());
+      }
+      scan(tryStatementTree.catches());
+      scan(tryStatementTree.finallyBlock());
+    }
   }
 
   private static class BlockVisitor extends BaseTreeVisitor {
@@ -185,7 +213,7 @@ public void visitMethodInvocation(MethodInvocationTree tree) {
 
     @Override
     public void visitThrowStatement(ThrowStatementTree tree) {
-      if (threadInterrupted || INTERRUPTING_TYPE_PREDICATE.test(tree.expression().symbolType())) {
+      if (threadInterrupted || isInterruptingExceptionExpression(tree.expression())) {
         threadInterrupted = true;
       } else {
         super.visitThrowStatement(tree);
@@ -202,4 +230,5 @@ public void visitLambdaExpression(LambdaExpressionTree tree) {
       // Cut visit on lambdas, because we only want to analyze actual control flow.
     }
   }
+
 }

java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java

@@ -139,7 +139,7 @@ static boolean hasKind(@Nullable Tree tree, Tree.Kind kind) {
   }
 
   public static class MethodInvocationCollector extends BaseTreeVisitor {
-    private final List<Tree> invocationTree = new ArrayList<>();
+    protected final List<Tree> invocationTree = new ArrayList<>();
     private final Predicate<Symbol.MethodSymbol> collectPredicate;
 
     public MethodInvocationCollector(Predicate<Symbol.MethodSymbol> collectPredicate) {