From 399c541c9a4ede47876a4d4b144208f6f74df71a Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Wed, 18 Jan 2023 05:59:58 -0800 Subject: [PATCH] Include stack trace in all gRPC errors when --verbose_failures is set. 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 --- .../build/lib/remote/RemoteModule.java | 3 ++- .../build/lib/remote/RemoteSpawnCache.java | 9 +------ .../build/lib/remote/RemoteSpawnRunner.java | 7 +---- .../devtools/build/lib/remote/util/Utils.java | 2 +- .../devtools/build/lib/remote/UtilsTest.java | 27 ++++++++++++++++--- 5 files changed, 29 insertions(+), 19 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java index 2795b133a1224f..18283b6b9c2774 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteModule.java @@ -537,7 +537,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); diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java index fd4205bf2925c0..6207d11edce643 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java @@ -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; @@ -141,13 +140,7 @@ public CacheHandle lookup(Spawn spawn, SpawnExecutionContext context) if (BulkTransferException.allCausedByCacheNotFoundException(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(); } diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java index ec7e7d7e86d6e2..4be0774599b770 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java @@ -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; @@ -573,11 +572,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(); diff --git a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java index 720560b8e925af..4ca78d304adb7a 100644 --- a/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java +++ b/src/main/java/com/google/devtools/build/lib/remote/util/Utils.java @@ -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. diff --git a/src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java b/src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java index 6dcb2d53fdcb8b..c99d88bb2b9d3c 100644 --- a/src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java +++ b/src/test/java/com/google/devtools/build/lib/remote/UtilsTest.java @@ -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