Skip to content

Commit

Permalink
Better implicate an OOM in BugReport#logException message.
Browse files Browse the repository at this point in the history 
PiperOrigin-RevId: 511793784
Change-Id: I962979620b82bc7b655a61cf09b28af2bf1b7631
  • Loading branch information
@copybara-github
Googler authored and copybara-github committed Feb 23, 2023
1 parent ecdb17d commit e2597ca
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 28 deletions.
4 changes: 3 additions & 1 deletion src/main/java/com/google/devtools/build/lib/bugreport/BugReport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -410,7 +411,8 @@ private static ImmutableList<String> filterArgs(Iterable<String> args) {
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: ";
Expand Down
7 changes: 6 additions & 1 deletion src/main/java/com/google/devtools/build/lib/util/CrashFailureDetails.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,11 @@ public static void setOomDetector(BooleanSupplier oomDetector) {
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));
}
Expand All @@ -67,7 +72,7 @@ public static FailureDetail forThrowable(Throwable throwable) {
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 {
Expand Down
1 change: 1 addition & 0 deletions src/test/java/com/google/devtools/build/lib/bugreport/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ java_library(
"//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",
Expand Down
105 changes: 79 additions & 26 deletions src/test/java/com/google/devtools/build/lib/bugreport/BugReportTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand All @@ -80,13 +86,27 @@ public static void installCustomSecurityManager() {
}
}

@Before
public void maybeSetOomDetector() {
if (oomDetectorOverride) {
CrashFailureDetails.setOomDetector(() -> true);
}
}

@AfterClass
public static void uninstallCustomSecurityManager() {
if (System.getSecurityManager() instanceof ExitProhibitingSecurityManager) {
System.setSecurityManager(null);
}
}

@After
public void restoreDefaultOomDetector() {
if (oomDetectorOverride) {
CrashFailureDetails.setOomDetector(() -> false);
}
}

private enum CrashType {
CRASH(ExitCode.BLAZE_INTERNAL_ERROR, Code.CRASH_UNKNOWN) {
@Override
Expand Down Expand Up @@ -145,6 +165,14 @@ private enum ExceptionType {
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();
Expand Down Expand Up @@ -173,27 +201,29 @@ public void resetPublishers() {
}

@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);
LoggingUtil.installRemoteLoggerForTesting(immediateFuture(logger));

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) {
Expand All @@ -203,16 +233,22 @@ public void convenienceMethod(@TestParameter CrashType crashType) throws Excepti
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);
}

@Test
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 =
Expand All @@ -226,22 +262,31 @@ public void halt(@TestParameter CrashType crashType) throws Exception {
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);
}

@Test
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);
}
Expand All @@ -260,8 +305,8 @@ public void customContext_setUpFront(@TestParameter CrashType crashType) {
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!");
Expand All @@ -288,7 +333,8 @@ public void customContext_filledInByRuntime(@TestParameter CrashType crashType)
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!");
Expand All @@ -311,19 +357,26 @@ private void verifyFailureDetailWritten(FailureDetail expected) throws Exception
}

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();
}

Expand Down

0 comments on commit e2597ca

Please sign in to comment.