Skip to content

Commit

Permalink
Include stack trace in all gRPC errors when --verbose_failures is set.
Browse files Browse the repository at this point in the history
Also refactor a couple places where the stack trace was included in an ad-hoc
manner, and force Utils.grpcAwareErrorMessage callers to be explicit to avoid
future instances.

Closes #16086.

PiperOrigin-RevId: 502854490
Change-Id: Id2d6a1728630fffea9399b406378c7f173b247bd
  • Loading branch information
tjgq authored and hvadehra committed Feb 14, 2023
1 parent 1e2cd3b commit b530baa
Show file tree
Hide file tree
Showing 5 changed files with 29 additions and 19 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,8 @@ public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
}
} catch (IOException e) {
String errorMessage =
"Failed to query remote execution capabilities: " + Utils.grpcAwareErrorMessage(e);
"Failed to query remote execution capabilities: "
+ Utils.grpcAwareErrorMessage(e, verboseFailures);
if (remoteOptions.remoteLocalFallback) {
if (verboseFailures) {
errorMessage += System.lineSeparator() + Throwables.getStackTraceAsString(e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@

import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Stopwatch;
import com.google.common.base.Throwables;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.FileArtifactValue;
Expand Down Expand Up @@ -141,13 +140,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context)
if (BulkTransferException.isOnlyCausedByCacheNotFoundException(e)) {
// Intentionally left blank
} else {
String errorMessage;
if (!verboseFailures) {
errorMessage = Utils.grpcAwareErrorMessage(e);
} else {
// On --verbose_failures print the whole stack trace
errorMessage = "\n" + Throwables.getStackTraceAsString(e);
}
String errorMessage = Utils.grpcAwareErrorMessage(e, verboseFailures);
if (isNullOrEmpty(errorMessage)) {
errorMessage = e.getClass().getSimpleName();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Preconditions;
import com.google.common.base.Stopwatch;
import com.google.common.base.Throwables;
import com.google.common.util.concurrent.ListeningScheduledExecutorService;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.CommandLines.ParamFileActionInput;
Expand Down Expand Up @@ -566,11 +565,7 @@ private SpawnResult handleError(
catastrophe = false;
}

String errorMessage = Utils.grpcAwareErrorMessage(exception);
if (verboseFailures) {
// On --verbose_failures print the whole stack trace
errorMessage += "\n" + Throwables.getStackTraceAsString(exception);
}
String errorMessage = Utils.grpcAwareErrorMessage(exception, verboseFailures);

if (exception.getCause() instanceof ExecutionStatusException) {
ExecutionStatusException e = (ExecutionStatusException) exception.getCause();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ private static String executionStatusExceptionErrorMessage(ExecutionStatusExcept
+ errorDetailsMessage(status.getDetailsList());
}

public static String grpcAwareErrorMessage(IOException e) {
private static String grpcAwareErrorMessage(IOException e) {
io.grpc.Status errStatus = io.grpc.Status.fromThrowable(e);
if (e.getCause() instanceof ExecutionStatusException) {
// Display error message returned by the remote service.
Expand Down
27 changes: 24 additions & 3 deletions src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,35 @@
public class UtilsTest {

@Test
public void testGrpcAwareErrorMessages() {
public void testGrpcAwareErrorMessage() {
IOException ioError = new IOException("io error");
IOException wrappedGrpcError =
new IOException(
"wrapped error", Status.ABORTED.withDescription("grpc error").asRuntimeException());

assertThat(Utils.grpcAwareErrorMessage(ioError)).isEqualTo("io error");
assertThat(Utils.grpcAwareErrorMessage(wrappedGrpcError)).isEqualTo("ABORTED: grpc error");
assertThat(Utils.grpcAwareErrorMessage(ioError, /* verboseFailures= */ false))
.isEqualTo("io error");
assertThat(Utils.grpcAwareErrorMessage(wrappedGrpcError, /* verboseFailures= */ false))
.isEqualTo("ABORTED: grpc error");
}

@Test
public void testGrpcAwareErrorMessage_verboseFailures() {
IOException ioError = new IOException("io error");
IOException wrappedGrpcError =
new IOException(
"wrapped error", Status.ABORTED.withDescription("grpc error").asRuntimeException());

assertThat(Utils.grpcAwareErrorMessage(ioError, /* verboseFailures= */ true))
.startsWith(
"io error\n"
+ "java.io.IOException: io error\n"
+ "\tat com.google.devtools.build.lib.remote.UtilsTest.testGrpcAwareErrorMessage_verboseFailures");
assertThat(Utils.grpcAwareErrorMessage(wrappedGrpcError, /* verboseFailures= */ true))
.startsWith(
"ABORTED: grpc error\n"
+ "java.io.IOException: wrapped error\n"
+ "\tat com.google.devtools.build.lib.remote.UtilsTest.testGrpcAwareErrorMessage_verboseFailures");
}

@Test
Expand Down

0 comments on commit b530baa

Please sign in to comment.