Skip to content

Commit

Permalink
SONARJAVA-4406 FP on S2142 when the InterruptedException is caught in…
Browse files Browse the repository at this point in the history 
… an inner try-catch
  • Loading branch information
@kaufco @leveretka
kaufco authored and leveretka committed Feb 20, 2023
1 parent 4b51016 commit b1109b5
Show file tree
Hide file tree
Showing 3 changed files with 118 additions and 33 deletions.
106 changes: 81 additions & 25 deletions java-checks-test-sources/src/main/java/checks/InterruptedExceptionCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -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();
Expand All @@ -71,7 +71,7 @@ public void runInterrupted() {
// clean up state...
new Interruptable().interrupt();
}
}
}

public Object getNextTask(BlockingQueue<Object> queue) {
boolean interrupted = false;
Expand All @@ -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);
}
}

Expand All @@ -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 {}.");
Expand Down Expand Up @@ -275,7 +275,6 @@ public void throwNewInterruptedExceptionFromFunction() throws InterruptedExcepti
}

public void throwNewCustomizedInterruptedExceptionFromFunction() throws InterruptedException {

try {
throwsInterruptedException();
} catch (InterruptedException e) { // Compliant
Expand All @@ -302,7 +301,6 @@ public void throwNewCustomizedInterruptedExceptionFromFunction() throws Interrup
}

public void rethrowSameException() throws InterruptedException {

try {
throwsInterruptedException();
} catch (InterruptedException e) { // Compliant
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand Down Expand Up @@ -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();
}

Expand Down
43 changes: 36 additions & 7 deletions java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand All @@ -202,4 +230,5 @@ public void visitLambdaExpression(LambdaExpressionTree tree) {
// Cut visit on lambdas, because we only want to analyze actual control flow.
}
}

}
2 changes: 1 addition & 1 deletion java-checks/src/main/java/org/sonar/java/checks/helpers/MethodTreeUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down

0 comments on commit b1109b5

Please sign in to comment.