SONARJAVA-5802 New analysis parameter: sonar.java.failOnStackOverflow (true by default) (#5323)

Author: leonardo-pilastri-sonarsource

java-frontend/src/main/java/org/sonar/java/SonarComponents.java

@@ -102,6 +102,14 @@ public class SonarComponents extends CheckRegistrar.RegistrarContext {
    * relying on (transitive) dependencies that do not respect modularization as defined by the JLS.
    */
   public static final String SONAR_IGNORE_UNNAMED_MODULE_FOR_SPLIT_PACKAGE = "sonar.java.ignoreUnnamedModuleForSplitPackage";
+
+  /**
+   * Describes whether the analysis should fail and halt when a StackOverflowError is encountered.
+   * True by default, we still want to give the possibility to disable this behavior in case of large projects
+   * for which it is costly to run a whole analysis to then just fail at the end and lose all findings.
+   */
+  public static final String SONAR_FAIL_ON_STACKOVERFLOW = "sonar.java.internal.failOnStackOverflow";
+
   private static final Version SONARLINT_6_3 = Version.parse("6.3");
   private static final Version SONARQUBE_9_2 = Version.parse("9.2");
   @VisibleForTesting
@@ -492,6 +500,10 @@ public boolean shouldIgnoreUnnamedModuleForSplitPackage() {
     return context.config().getBoolean(SONAR_IGNORE_UNNAMED_MODULE_FOR_SPLIT_PACKAGE).orElse(false);
   }
 
+  public boolean shouldFailOnStackOverflow() {
+    return context.config().getBoolean(SONAR_FAIL_ON_STACKOVERFLOW).orElse(true);
+  }
+
   private static long computeIdealBatchSize() {
     // We take a fraction of the total memory available though -Xmx.
     // If we assume that the average size of a file is 5KB and the average CI should have 1GB of memory,

java-frontend/src/main/java/org/sonar/java/ast/JavaAstScanner.java

@@ -187,7 +187,9 @@ public void simpleScan(InputFile inputFile, JParserConfig.Result result, Consume
       interruptIfFailFast(e, inputFile);
     } catch (StackOverflowError error) {
       LOG.error(String.format(LOG_ERROR_STACKOVERFLOW, inputFile), error);
-      throw error;
+      if (sonarComponents == null || sonarComponents.shouldFailOnStackOverflow()) {
+        throw error;
+      }
     } finally {
       telemetry.aggregateAsCounter(telemetryAnalysisKeys.sizeCharsKey(), InputFileUtils.charCount(inputFile, 0));
       telemetry.aggregateAsCounter(telemetryAnalysisKeys.timeMsKey(), currentTimeMillis() - startTime);

java-frontend/src/test/java/org/sonar/java/SonarComponentsTest.java

@@ -41,6 +41,7 @@
 import org.junit.jupiter.params.provider.Arguments;
 import org.junit.jupiter.params.provider.CsvSource;
 import org.junit.jupiter.params.provider.MethodSource;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mock;
 import org.mockito.Mockito;
 import org.mockito.junit.jupiter.MockitoExtension;
@@ -1064,6 +1065,23 @@ void shouldIgnoreUnnamedModuleForSplitPackage_returns_true_when_analysis_paramet
     assertThat(sonarComponents.shouldIgnoreUnnamedModuleForSplitPackage()).isTrue();
   }
 
+  @Test
+  void shouldFailOnStackOverflow_returns_true_by_default() {
+    MapSettings settings = new MapSettings();
+    SonarComponents sonarComponents = new SonarComponents(null, null, null, null, null, null);
+    sonarComponents.setSensorContext(SensorContextTester.create(new File("")).setSettings(settings));
+    assertThat(sonarComponents.shouldFailOnStackOverflow()).isTrue();
+  }
+
+  @ParameterizedTest
+  @ValueSource(strings = {"true", "false"})
+  void shouldFailOnStackOverflow_returns_correct_value_when_set(String value) {
+    MapSettings settings = new MapSettings().setProperty("sonar.java.internal.failOnStackOverflow", value);
+    SonarComponents sonarComponents = new SonarComponents(null, null, null, null, null, null);
+    sonarComponents.setSensorContext(SensorContextTester.create(new File("")).setSettings(settings));
+    assertThat(sonarComponents.shouldFailOnStackOverflow()).isEqualTo(Boolean.valueOf(value));
+  }
+
   @Nested
   class Logging {
     private final DecimalFormat formatter = new DecimalFormat("00");

java-frontend/src/test/java/org/sonar/java/ast/JavaAstScannerTest.java

@@ -29,6 +29,7 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.junit.jupiter.params.provider.ValueSource;
 import org.slf4j.event.Level;
 import org.sonar.api.batch.fs.InputFile;
@@ -67,6 +68,7 @@
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.Assertions.fail;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.doReturn;
@@ -81,6 +83,11 @@
 
 class JavaAstScannerTest {
 
+  public static final InputFile TRIVIAL_COMPILATION_UNIT = TestUtils.inputFile("src/test/resources/AstScannerNoParseError.txt");
+  public static final JavaFileScanner PROBLEMATIC_VISITOR = ctx -> {
+    throw new StackOverflowError();
+  };
+
   @RegisterExtension
   public ThreadLocalLogTester logTester = new ThreadLocalLogTester().setLevel(Level.DEBUG);
 
@@ -450,7 +457,6 @@ void scanWithoutParsing_filters_out_the_files_that_could_be_successfully_scanned
 
   @Test
   void test_modifyCompilationUnit_modify_ast() {
-    InputFile trivialCompilationUnit = TestUtils.inputFile("src/test/resources/AstScannerNoParseError.txt");
 
     var check = new JavaFileScanner() {
       @Override
@@ -468,7 +474,7 @@ public void scanFile(JavaFileScannerContext context) {
 
     JavaAstScanner scanner = new JavaAstScanner(null, new NoOpTelemetry(), TelemetryKey.JAVA_ANALYSIS_MAIN);
     scanner.setVisitorBridge(visitorsBridge);
-    scanner.scanForTesting(Collections.singletonList(trivialCompilationUnit), compilationUnit -> {
+    scanner.scanForTesting(Collections.singletonList(TRIVIAL_COMPILATION_UNIT), compilationUnit -> {
       var clazz = (ClassTreeImpl) ((JavaTree.CompilationUnitTreeImpl) compilationUnit).types().get(0);
       assertThat(clazz.simpleName().name()).isEqualTo("C");
       assertThat(clazz.simpleName().symbol().isUnknown()).isFalse();
@@ -478,6 +484,59 @@ public void scanFile(JavaFileScannerContext context) {
 
   }
 
+  @Test
+  void test_should_fail_on_stackoverflow_by_default() {
+    var files = List.of(TRIVIAL_COMPILATION_UNIT);
+    List<JavaFileScanner> visitors = List.of(PROBLEMATIC_VISITOR);
+
+    // Assert that by default, StackOverflowError is propagated
+    var emptySettings = new MapSettings();
+    assertThrows(StackOverflowError.class, () -> scanFilesWithVisitorsAndContext(files, visitors, emptySettings));
+  }
+
+  @Test
+  void test_should_not_fail_on_stackoverflow_when_set() {
+    // Assert that when configured to not fail on exception, StackOverflowError is swallowed, but logged
+    MapSettings settings = new MapSettings().setProperty(SonarComponents.SONAR_FAIL_ON_STACKOVERFLOW, false);
+    assertDoesNotThrow(() -> scanFilesWithVisitorsAndContext(List.of(TRIVIAL_COMPILATION_UNIT), List.of(PROBLEMATIC_VISITOR), settings));
+    assertThat(logTester.logs(Level.ERROR))
+      .containsExactly("A stack overflow error occurred while analyzing file: 'src/test/resources/AstScannerNoParseError.txt'");
+  }
+
+  @ParameterizedTest
+  @MethodSource("failFastAndFailOnStackOverflowCombinations")
+  void assertFailFastWithFailOnStackOverflowInteraction(boolean failFast, boolean failOnSO, boolean shouldFail) {
+    MapSettings settings = new MapSettings()
+      .setProperty(SonarComponents.FAIL_ON_EXCEPTION_KEY, failFast)
+      .setProperty(SonarComponents.SONAR_FAIL_ON_STACKOVERFLOW, failOnSO);
+    var files = List.of(TRIVIAL_COMPILATION_UNIT);
+    List<JavaFileScanner> visitors = List.of(PROBLEMATIC_VISITOR);
+
+    if (shouldFail) {
+      assertThrows(StackOverflowError.class, () -> scanFilesWithVisitorsAndContext(files, visitors, settings));
+    } else {
+      assertDoesNotThrow(() -> scanFilesWithVisitorsAndContext(files, visitors, settings));
+    }
+  }
+
+  static List<Object[]> failFastAndFailOnStackOverflowCombinations() {
+    return Arrays.asList(new Object[][]{
+      {true, true, true},
+      {true, false, false},
+      {false, true, true},
+      {false, false, false}
+    });
+  }
+
+  @Test
+  void test_should_fail_on_stackoverflow_when_sonar_components_null() {
+    var files = List.of(TRIVIAL_COMPILATION_UNIT);
+    JavaAstScanner scanner = new JavaAstScanner(null, new NoOpTelemetry(), TelemetryKey.JAVA_ANALYSIS_MAIN);
+    VisitorsBridge visitorBridge = new VisitorsBridge(PROBLEMATIC_VISITOR);
+    scanner.setVisitorBridge(visitorBridge);
+    assertThrows(StackOverflowError.class, () -> scanner.scan(files));
+  }
+
   private void scanSingleFile(InputFile file, boolean failOnException) {
     scanFilesWithVisitors(Collections.singletonList(file), Collections.emptyList(), -1, failOnException, false);
   }
@@ -501,19 +560,27 @@ private void scanWithJavaVersion(int version, List<InputFile> inputFiles, List<J
   }
 
   private void scanFilesWithVisitors(List<InputFile> inputFiles, List<JavaFileScanner> visitors,
-                                     int javaVersion, boolean failOnException, boolean autoscanMode) {
-    context.setSettings(new MapSettings()
+    int javaVersion, boolean failOnException, boolean autoscanMode) {
+    MapSettings settings = new MapSettings()
       .setProperty(SonarComponents.FAIL_ON_EXCEPTION_KEY, failOnException)
-      .setProperty(SonarComponents.SONAR_AUTOSCAN, autoscanMode)
-    );
+      .setProperty(SonarComponents.SONAR_AUTOSCAN, autoscanMode);
+
+    scanFilesWithVisitorsAndContext(inputFiles, visitors, settings, javaVersion);
+  }
+
+  private void scanFilesWithVisitorsAndContext(List<InputFile> inputFiles, List<JavaFileScanner> visitors, MapSettings contextMap) {
+    scanFilesWithVisitorsAndContext(inputFiles, visitors, contextMap, JavaVersionImpl.MAX_SUPPORTED);
+  }
 
+  private void scanFilesWithVisitorsAndContext(List<InputFile> inputFiles, List<JavaFileScanner> visitors, MapSettings contextMap, int javaVersion) {
+    context.setSettings(contextMap);
     DefaultFileSystem fileSystem = context.fileSystem();
     ClasspathForMain classpathForMain = new ClasspathForMain(context.config(), fileSystem);
     ClasspathForTest classpathForTest = new ClasspathForTest(context.config(), fileSystem);
     SonarComponents sonarComponents = new SonarComponents(null, fileSystem, classpathForMain, classpathForTest, null, null);
     sonarComponents.setSensorContext(context);
     JavaAstScanner scanner = new JavaAstScanner(sonarComponents, new NoOpTelemetry(), TelemetryKey.JAVA_ANALYSIS_MAIN);
-    VisitorsBridge visitorBridge = new VisitorsBridge(visitors, new ArrayList<>(), sonarComponents, new JavaVersionImpl(javaVersion));
+    VisitorsBridge visitorBridge = new VisitorsBridge(visitors, List.of(), sonarComponents, new JavaVersionImpl(javaVersion));
     scanner.setVisitorBridge(visitorBridge);
     scanner.scan(inputFiles);
   }

sonar-java-plugin/src/main/resources/static/documentation.md

@@ -101,6 +101,13 @@ You can control this behavior with the analysis parameter `sonar.java.skipUnchan
 * setting it to `false` will **never** make rules skip unchanged files, even if the context is a PR analysis
 * not setting this parameter lets the server decide whether the optimization should be enabled, by default it will be enabled for PR analyses.
 
+## Handling analysis runtime errors
+By default, the Java analyzer only halts the analysis when an `AnalysisException` or a `java.lang.Error` (non-recoverable error) is thrown.
+Generic runtime exceptions (like `NullPointerException`, `ArrayIndexOutOfBoundsException`, etc.) are caught and logged but do not stop the analysis.
+This behavior can be changed by setting two different analysis parameters:
+* `sonar.internal.analysis.failFast` - when set to `true`, any runtime exception will halt the analysis
+* `sonar.java.internal.failOnStackOverflow` - `true` by default, can be set to `false` to avoid halting the analysis on `StackOverflowError` (this overrides the `failFast` option)
+
 ## Cache-enabled rules (experimental)
 Starting from April 2022, the Java analyzer offers rule developers a SQ cache that can be used to store and retrieve information from one analysis to the other.
 The cache is provided by the underlying SonarQube instance and is branch-specific.