Better implicate an OOM in `BugReport#logException` message. PiperOrigin-RevId: 511793784 Change-Id: I962979620b82bc7b655a61cf09b28af2bf1b7631
diff --git a/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java b/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java index 2915a23..d7bc9a5 100644 --- a/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java +++ b/src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java
@@ -24,6 +24,7 @@ import com.google.common.flogger.GoogleLogger; import com.google.devtools.build.lib.analysis.BlazeVersionInfo; import com.google.devtools.build.lib.events.Event; +import com.google.devtools.build.lib.util.CrashFailureDetails; import com.google.devtools.build.lib.util.CustomExitCodePublisher; import com.google.devtools.build.lib.util.CustomFailureDetailPublisher; import com.google.devtools.build.lib.util.DetailedExitCode; @@ -410,7 +411,8 @@ static void logException( Throwable exception, boolean isCrash, List<String> args, String... values) { logger.atSevere().withCause(exception).log("Exception"); - String preamble = getProductName(); + String preamble = + CrashFailureDetails.oomDetected() ? "While OOMing, " + getProductName() : getProductName(); Level level = isCrash ? Level.SEVERE : Level.WARNING; if (!isCrash) { preamble += " had a non fatal error with args: ";
diff --git a/src/main/java/com/google/devtools/build/lib/util/CrashFailureDetails.java b/src/main/java/com/google/devtools/build/lib/util/CrashFailureDetails.java index 7ea5394..5857b72 100644 --- a/src/main/java/com/google/devtools/build/lib/util/CrashFailureDetails.java +++ b/src/main/java/com/google/devtools/build/lib/util/CrashFailureDetails.java
@@ -58,6 +58,11 @@ CrashFailureDetails.oomDetector = oomDetector; } + /** Returns whether an {@link OutOfMemoryError} was detected. */ + public static boolean oomDetected() { + return oomDetector.getAsBoolean(); + } + public static DetailedExitCode detailedExitCodeForThrowable(Throwable throwable) { return DetailedExitCode.of(forThrowable(throwable)); } @@ -67,7 +72,7 @@ Crash.Builder crashBuilder = Crash.newBuilder(); if (getRootCauseToleratingCycles(throwable) instanceof OutOfMemoryError) { crashBuilder.setCode(Crash.Code.CRASH_OOM); - } else if (oomDetector.getAsBoolean()) { + } else if (oomDetected()) { logger.atWarning().log("Classifying non-OOM crash as OOM"); crashBuilder.setCode(Crash.Code.CRASH_OOM).setOomDetectorOverride(true); } else {
diff --git a/src/test/java/com/google/devtools/build/lib/bugreport/BUILD b/src/test/java/com/google/devtools/build/lib/bugreport/BUILD index 9a539b0..620cde1 100644 --- a/src/test/java/com/google/devtools/build/lib/bugreport/BUILD +++ b/src/test/java/com/google/devtools/build/lib/bugreport/BUILD
@@ -20,6 +20,7 @@ "//src/java_tools/junitrunner/java/com/google/testing/junit/runner/util", "//src/main/java/com/google/devtools/build/lib/bugreport", "//src/main/java/com/google/devtools/build/lib/events", + "//src/main/java/com/google/devtools/build/lib/util:crash_failure_details", "//src/main/java/com/google/devtools/build/lib/util:custom_exit_code_publisher", "//src/main/java/com/google/devtools/build/lib/util:custom_failure_detail_publisher", "//src/main/java/com/google/devtools/build/lib/util:detailed_exit_code",
diff --git a/src/test/java/com/google/devtools/build/lib/bugreport/BugReportTest.java b/src/test/java/com/google/devtools/build/lib/bugreport/BugReportTest.java index eaaf815..a61662d 100644 --- a/src/test/java/com/google/devtools/build/lib/bugreport/BugReportTest.java +++ b/src/test/java/com/google/devtools/build/lib/bugreport/BugReportTest.java
@@ -34,6 +34,7 @@ import com.google.devtools.build.lib.server.FailureDetails; import com.google.devtools.build.lib.server.FailureDetails.Crash.Code; import com.google.devtools.build.lib.server.FailureDetails.FailureDetail; +import com.google.devtools.build.lib.util.CrashFailureDetails; import com.google.devtools.build.lib.util.CustomExitCodePublisher; import com.google.devtools.build.lib.util.CustomFailureDetailPublisher; import com.google.devtools.build.lib.util.DetailedExitCode; @@ -71,6 +72,11 @@ @RunWith(TestParameterInjector.class) public final class BugReportTest { + private static final ExitCode EXIT_CODE_BLAZE_OOMING = ExitCode.OOM_ERROR; + private static final Code FAILURE_DETAIL_CODE_BLAZE_OOMING = Code.CRASH_OOM; + + @TestParameter private boolean oomDetectorOverride; + @BeforeClass public static void installCustomSecurityManager() { if (System.getSecurityManager() == null) { @@ -80,6 +86,13 @@ } } + @Before + public void maybeSetOomDetector() { + if (oomDetectorOverride) { + CrashFailureDetails.setOomDetector(() -> true); + } + } + @AfterClass public static void uninstallCustomSecurityManager() { if (System.getSecurityManager() instanceof ExitProhibitingSecurityManager) { @@ -87,6 +100,13 @@ } } + @After + public void restoreDefaultOomDetector() { + if (oomDetectorOverride) { + CrashFailureDetails.setOomDetector(() -> false); + } + } + private enum CrashType { CRASH(ExitCode.BLAZE_INTERNAL_ERROR, Code.CRASH_UNKNOWN) { @Override @@ -145,6 +165,14 @@ this.level = level; this.expectedMessage = expectedMessage; } + + private String getExpectedMessage() { + return expectedMessage; + } + + private String getExpectedMessageWhileOoming() { + return "While OOMing, " + expectedMessage; + } } @Rule public final TemporaryFolder tmp = new TemporaryFolder(); @@ -173,7 +201,7 @@ } @Test - public void logException(@TestParameter ExceptionType exceptionType) throws Exception { + public void logException(@TestParameter ExceptionType exceptionType) { TestLogHandler handler = new TestLogHandler(); Logger logger = Logger.getLogger("build.lib.bugreport"); logger.addHandler(handler); @@ -181,19 +209,21 @@ BugReport.logException( exceptionType.throwable, exceptionType.isFatal, ImmutableList.of("arg", "foo")); - LogRecord got = handler.getStoredLogRecords().get(0); + if (oomDetectorOverride) { + assertThat(got.getMessage()).isEqualTo(exceptionType.getExpectedMessageWhileOoming()); + } else { + assertThat(got.getMessage()).isEqualTo(exceptionType.getExpectedMessage()); + } assertThat(got.getThrown()).isSameInstanceAs(exceptionType.throwable); assertThat(got.getLevel()).isEqualTo(exceptionType.level); - assertThat(got.getMessage()).isEqualTo(exceptionType.expectedMessage); } @Test public void convenienceMethod(@TestParameter CrashType crashType) throws Exception { Throwable t = crashType.createThrowable(); FailureDetail expectedFailureDetail = - createExpectedFailureDetail(t, crashType.expectedFailureDetailCode); - + createExpectedFailureDetail(t, crashType, oomDetectorOverride); // TODO(b/222158599): This should always be ExitException. SecurityException e = assertThrows(SecurityException.class, () -> BugReport.handleCrash(t)); if (e instanceof ExitException) { @@ -203,8 +233,14 @@ assertThat(BugReport.getAndResetLastCrashingThrowableIfInTest()).isSameInstanceAs(t); verify(mockRuntime) - .cleanUpForCrash(DetailedExitCode.of(crashType.expectedExitCode, expectedFailureDetail)); - verifyExitCodeWritten(crashType.expectedExitCode.getNumericExitCode()); + .cleanUpForCrash( + DetailedExitCode.of( + oomDetectorOverride ? EXIT_CODE_BLAZE_OOMING : crashType.expectedExitCode, + expectedFailureDetail)); + verifyExitCodeWritten( + oomDetectorOverride + ? EXIT_CODE_BLAZE_OOMING.getNumericExitCode() + : crashType.expectedExitCode.getNumericExitCode()); verifyFailureDetailWritten(expectedFailureDetail); } @@ -212,7 +248,7 @@ public void halt(@TestParameter CrashType crashType) throws Exception { Throwable t = crashType.createThrowable(); FailureDetail expectedFailureDetail = - createExpectedFailureDetail(t, crashType.expectedFailureDetailCode); + createExpectedFailureDetail(t, crashType, oomDetectorOverride); // TODO(b/222158599): This should always be ExitException. SecurityException e = @@ -226,8 +262,14 @@ assertThat(BugReport.getAndResetLastCrashingThrowableIfInTest()).isSameInstanceAs(t); verify(mockRuntime) - .cleanUpForCrash(DetailedExitCode.of(crashType.expectedExitCode, expectedFailureDetail)); - verifyExitCodeWritten(crashType.expectedExitCode.getNumericExitCode()); + .cleanUpForCrash( + DetailedExitCode.of( + oomDetectorOverride ? EXIT_CODE_BLAZE_OOMING : crashType.expectedExitCode, + expectedFailureDetail)); + verifyExitCodeWritten( + oomDetectorOverride + ? EXIT_CODE_BLAZE_OOMING.getNumericExitCode() + : crashType.expectedExitCode.getNumericExitCode()); verifyFailureDetailWritten(expectedFailureDetail); } @@ -235,13 +277,16 @@ public void keepAlive(@TestParameter CrashType crashType) throws Exception { Throwable t = crashType.createThrowable(); FailureDetail expectedFailureDetail = - createExpectedFailureDetail(t, crashType.expectedFailureDetailCode); + createExpectedFailureDetail(t, crashType, oomDetectorOverride); BugReport.handleCrash(Crash.from(t), CrashContext.keepAlive()); assertThat(BugReport.getAndResetLastCrashingThrowableIfInTest()).isSameInstanceAs(t); verify(mockRuntime) - .cleanUpForCrash(DetailedExitCode.of(crashType.expectedExitCode, expectedFailureDetail)); + .cleanUpForCrash( + DetailedExitCode.of( + oomDetectorOverride ? EXIT_CODE_BLAZE_OOMING : crashType.expectedExitCode, + expectedFailureDetail)); verifyNoExitCodeWritten(); verifyFailureDetailWritten(expectedFailureDetail); } @@ -260,8 +305,8 @@ verify(handler).handle(event.capture()); assertThat(event.getValue().getKind()).isEqualTo(EventKind.FATAL); assertThat(event.getValue().getMessage()).contains(Throwables.getStackTraceAsString(t)); - - if (crashType == CrashType.OOM) { + if (oomDetectorOverride || crashType == CrashType.OOM) { + assertThat(event.getValue().getMessage()).contains("ran out of memory and crashed."); assertThat(event.getValue().getMessage()).contains("Build fewer targets!"); } else { assertThat(event.getValue().getMessage()).doesNotContain("Build fewer targets!"); @@ -288,7 +333,8 @@ assertThat(event.getValue().getKind()).isEqualTo(EventKind.FATAL); assertThat(event.getValue().getMessage()).contains(Throwables.getStackTraceAsString(t)); - if (crashType == CrashType.OOM) { + if (oomDetectorOverride || crashType == CrashType.OOM) { + assertThat(event.getValue().getMessage()).contains("ran out of memory and crashed."); assertThat(event.getValue().getMessage()).contains("Build fewer targets!"); } else { assertThat(event.getValue().getMessage()).doesNotContain("Build fewer targets!"); @@ -311,19 +357,26 @@ } private static FailureDetail createExpectedFailureDetail( - Throwable t, Code expectedFailureDetailCode) { + Throwable t, CrashType crashType, boolean oomDetectorOverride) { + FailureDetails.Crash.Builder crash = + FailureDetails.Crash.newBuilder() + .setCode( + oomDetectorOverride + ? FAILURE_DETAIL_CODE_BLAZE_OOMING + : crashType.expectedFailureDetailCode) + .addCauses( + FailureDetails.Throwable.newBuilder() + .setThrowableClass(t.getClass().getName()) + .setMessage(t.getMessage()) + .addAllStackTrace( + Lists.transform( + Arrays.asList(t.getStackTrace()), StackTraceElement::toString))); + if (oomDetectorOverride && crashType == CrashType.CRASH) { + crash.setOomDetectorOverride(true); + } return FailureDetail.newBuilder() .setMessage(String.format("Crashed: (%s) %s", t.getClass().getName(), t.getMessage())) - .setCrash( - FailureDetails.Crash.newBuilder() - .setCode(expectedFailureDetailCode) - .addCauses( - FailureDetails.Throwable.newBuilder() - .setThrowableClass(t.getClass().getName()) - .setMessage(t.getMessage()) - .addAllStackTrace( - Lists.transform( - Arrays.asList(t.getStackTrace()), StackTraceElement::toString)))) + .setCrash(crash) .build(); }