Document FailureDetail message, introduce Interrupted
This documents the conventions guiding forward compatible use of
FailureDetails by consumers.
To have something basic to illustrate those conventions, this implements
the Interrupted category and makes use of it in one place,
GrpcServerImpl.
RELNOTES: None.
PiperOrigin-RevId: 294513459
diff --git a/src/main/java/com/google/devtools/build/lib/BUILD b/src/main/java/com/google/devtools/build/lib/BUILD
index 0ea5446..b6bf332 100644
--- a/src/main/java/com/google/devtools/build/lib/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/BUILD
@@ -233,6 +233,7 @@
"util/BlazeClock.java",
"util/Clock.java",
"util/ExitCode.java",
+ "util/FailureDetailUtil.java",
"util/FileType.java",
"util/FileTypeSet.java",
"util/JavaClock.java",
@@ -319,6 +320,7 @@
],
deps = [
"//third_party:guava",
+ "//third_party:jsr305",
],
)
@@ -328,6 +330,17 @@
deps = ["//third_party:jsr305"],
)
+java_library(
+ name = "failure_detail_util",
+ srcs = ["util/FailureDetailUtil.java"],
+ deps = [
+ ":exitcode-external",
+ "//src/main/protobuf:failure_details_java_proto",
+ "//third_party:guava",
+ "//third_party/protobuf:protobuf_java",
+ ],
+)
+
# Event reporting infrastructure.
java_library(
name = "events",
@@ -1172,6 +1185,7 @@
deps = [
":bug-report",
":exitcode-external",
+ ":failure_detail_util",
":os_util",
":out-err",
":runtime",
@@ -1244,6 +1258,7 @@
":custom-exit-code-publisher",
":events",
":exitcode-external",
+ ":failure_detail_util",
":io",
":keep-going-option",
":loading-phase-threads-option",
@@ -1288,6 +1303,7 @@
"//src/main/protobuf:command_line_java_proto",
"//src/main/protobuf:command_server_java_proto",
"//src/main/protobuf:extra_actions_base_java_proto",
+ "//src/main/protobuf:failure_details_java_proto",
"//src/main/protobuf:invocation_policy_java_proto",
"//src/main/protobuf:option_filters_java_proto",
"//src/main/protobuf:test_status_java_proto",
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandResult.java b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandResult.java
index 8be6f9f..da17258 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandResult.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/BlazeCommandResult.java
@@ -18,7 +18,9 @@
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.server.CommandProtos.ExecRequest;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
import com.google.devtools.build.lib.util.ExitCode;
+import com.google.devtools.build.lib.util.FailureDetailUtil;
import javax.annotation.Nullable;
/**
@@ -28,12 +30,17 @@
@Immutable
public final class BlazeCommandResult {
private final ExitCode exitCode;
- @Nullable
- private final ExecRequest execDescription;
+ @Nullable private final FailureDetail failureDetail;
+ @Nullable private final ExecRequest execDescription;
private final boolean shutdown;
- private BlazeCommandResult(ExitCode exitCode, ExecRequest execDescription, boolean shutdown) {
+ private BlazeCommandResult(
+ ExitCode exitCode,
+ @Nullable FailureDetail failureDetail,
+ ExecRequest execDescription,
+ boolean shutdown) {
this.exitCode = Preconditions.checkNotNull(exitCode);
+ this.failureDetail = failureDetail;
this.execDescription = execDescription;
this.shutdown = shutdown;
}
@@ -42,6 +49,11 @@
return exitCode;
}
+ @Nullable
+ public FailureDetail getFailureDetail() {
+ return failureDetail;
+ }
+
public boolean shutdown() {
return shutdown;
}
@@ -55,21 +67,27 @@
public String toString() {
return MoreObjects.toStringHelper(this)
.add("exitCode", exitCode)
+ .add("failureDetail", failureDetail)
.add("execDescription", execDescription)
.add("shutdown", shutdown)
.toString();
}
public static BlazeCommandResult shutdown(ExitCode exitCode) {
- return new BlazeCommandResult(exitCode, null, true);
+ return new BlazeCommandResult(exitCode, null, null, true);
}
public static BlazeCommandResult exitCode(ExitCode exitCode) {
- return new BlazeCommandResult(exitCode, null, false);
+ return new BlazeCommandResult(exitCode, null, null, false);
+ }
+
+ public static BlazeCommandResult failureDetail(FailureDetail failureDetail) {
+ return new BlazeCommandResult(
+ FailureDetailUtil.getExitCode(failureDetail), failureDetail, null, false);
}
public static BlazeCommandResult execute(ExecRequest execDescription) {
return new BlazeCommandResult(
- ExitCode.SUCCESS, Preconditions.checkNotNull(execDescription), false);
+ ExitCode.SUCCESS, null, Preconditions.checkNotNull(execDescription), false);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
index 6276d4e..e831e62 100644
--- a/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
+++ b/src/main/java/com/google/devtools/build/lib/server/GrpcServerImpl.java
@@ -36,7 +36,9 @@
import com.google.devtools.build.lib.server.CommandProtos.RunResponse;
import com.google.devtools.build.lib.server.CommandProtos.ServerInfo;
import com.google.devtools.build.lib.server.CommandProtos.StartupOption;
+import com.google.devtools.build.lib.server.FailureDetails.Interrupted.InterruptedCode;
import com.google.devtools.build.lib.util.ExitCode;
+import com.google.devtools.build.lib.util.FailureDetailUtil;
import com.google.devtools.build.lib.util.Pair;
import com.google.devtools.build.lib.util.io.OutErr;
import com.google.devtools.build.lib.vfs.FileSystemUtils;
@@ -611,7 +613,9 @@
result = BlazeCommandResult.exitCode(ExitCode.COMMAND_LINE_ERROR);
}
} catch (InterruptedException e) {
- result = BlazeCommandResult.exitCode(ExitCode.INTERRUPTED);
+ result =
+ BlazeCommandResult.failureDetail(
+ FailureDetailUtil.interrupted(InterruptedCode.UNSPECIFIED));
commandId = ""; // The default value, the client will ignore it
}
@@ -626,6 +630,9 @@
response.setExecRequest(result.getExecRequest());
} else {
response.setExitCode(result.getExitCode().getNumericExitCode());
+ if (result.getFailureDetail() != null) {
+ response.setFailureDetail(result.getFailureDetail());
+ }
}
try {
diff --git a/src/main/java/com/google/devtools/build/lib/util/ExitCode.java b/src/main/java/com/google/devtools/build/lib/util/ExitCode.java
index 34b7bbb..fda79d9 100644
--- a/src/main/java/com/google/devtools/build/lib/util/ExitCode.java
+++ b/src/main/java/com/google/devtools/build/lib/util/ExitCode.java
@@ -17,6 +17,7 @@
import com.google.common.base.Objects;
import java.util.Collection;
import java.util.HashMap;
+import javax.annotation.Nullable;
/**
* <p>Anything marked FAILURE is generally from a problem with the source code
@@ -134,6 +135,18 @@
}
}
+ /**
+ * Returns a registered {@link ExitCode} with the given {@code code}.
+ *
+ * <p>Note that there *are* unregistered ExitCodes. This will never return them.
+ */
+ @Nullable
+ static ExitCode forCode(int code) {
+ synchronized (exitCodeRegistry) {
+ return exitCodeRegistry.get(code);
+ }
+ }
+
private final int numericExitCode;
private final String name;
private final boolean infrastructureFailure;
diff --git a/src/main/java/com/google/devtools/build/lib/util/FailureDetailUtil.java b/src/main/java/com/google/devtools/build/lib/util/FailureDetailUtil.java
new file mode 100644
index 0000000..4aec6eb
--- /dev/null
+++ b/src/main/java/com/google/devtools/build/lib/util/FailureDetailUtil.java
@@ -0,0 +1,68 @@
+// Copyright 2020 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.util;
+
+import com.google.common.base.Preconditions;
+import com.google.devtools.build.lib.server.FailureDetails;
+import com.google.devtools.build.lib.server.FailureDetails.FailureDetail;
+import com.google.devtools.build.lib.server.FailureDetails.Interrupted;
+import com.google.devtools.build.lib.server.FailureDetails.Interrupted.InterruptedCode;
+import com.google.protobuf.ProtocolMessageEnum;
+
+/** Utilities for constructing and managing {@link FailureDetail}s. */
+public class FailureDetailUtil {
+
+ private FailureDetailUtil() {}
+
+ /**
+ * Returns a {@link FailureDetail} specifying {@code code} in its {@link Interrupted} submessage.
+ */
+ public static FailureDetail interrupted(InterruptedCode code) {
+ return FailureDetail.newBuilder()
+ .setInterrupted(Interrupted.newBuilder().setCode(code))
+ .build();
+ }
+
+ /** Returns the registered {@link ExitCode} associated with a {@link FailureDetail} message. */
+ public static ExitCode getExitCode(FailureDetail failureDetail) {
+ // TODO(mschaller): Consider specializing for unregistered exit codes here, if absolutely
+ // necessary.
+ int numericExitCode = FailureDetailUtil.getNumericExitCode(failureDetail);
+ return Preconditions.checkNotNull(
+ ExitCode.forCode(numericExitCode), "No ExitCode for numericExitCode %s", numericExitCode);
+ }
+
+ /** Returns the numeric exit code associated with a {@link FailureDetail} message. */
+ private static int getNumericExitCode(FailureDetail failureDetail) {
+ // TODO(mschaller): generalize this for arbitrary FailureDetail messages.
+ Preconditions.checkArgument(failureDetail.hasInterrupted());
+ return getNumericExitCode(failureDetail.getInterrupted().getCode());
+ }
+
+ /**
+ * Returns the numeric exit code associated with a {@link FailureDetail} submessage's subcategory
+ * enum value.
+ */
+ private static int getNumericExitCode(ProtocolMessageEnum code) {
+ Preconditions.checkArgument(
+ code.getValueDescriptor().getOptions().hasExtension(FailureDetails.metadata),
+ "Enum value %s has no FailureDetails.metadata",
+ code);
+ return code.getValueDescriptor()
+ .getOptions()
+ .getExtension(FailureDetails.metadata)
+ .getExitCode();
+ }
+}
diff --git a/src/main/protobuf/failure_details.proto b/src/main/protobuf/failure_details.proto
index f4095f1..79adce5 100644
--- a/src/main/protobuf/failure_details.proto
+++ b/src/main/protobuf/failure_details.proto
@@ -33,7 +33,71 @@
FailureDetailMetadata metadata = 1078;
}
+// The FailureDetail message type is designed such that consumers can extract a
+// basic classification of a FailureDetail message even if the consumer was
+// built with a stale definition. This forward compatibility is implemented via
+// conventions on FailureDetail and its submessage types, as follows.
+//
+// *** FailureDetail field numbers
+//
+// Field numbers 1 through 100 (inclusive) are reserved for generally applicable
+// values. Any number of these fields may be set on a FailureDetail message.
+//
+// Field numbers 101 through 10,000 (inclusive) are reserved for use inside the
+// "oneof" structure. Only one of these values should be set on a FailureDetail
+// message.
+//
+// Additional fields numbers are unlikely to be needed, but, for extreme future-
+// proofing purposes, field numbers 10,001 through 1,000,000 (inclusive;
+// excluding protobuf's reserved range 19000 through 19999) are reserved for
+// additional generally applicable values.
+//
+// *** FailureDetail's "oneof" submessages
+//
+// Each field in the "oneof" structure is a submessage corresponding to a
+// category of failure.
+//
+// In each of these submessage types, field number 1 is an enum whose values
+// correspond to a subcategory of the failure. Generally, the enum's constant
+// which maps to 0 should be interpreted as "unspecified", though this is not
+// required.
+//
+// *** Recommended forward compatibility strategy
+//
+// The recommended forward compatibility strategy is to reduce a FailureDetail
+// message to a pair of integers.
+//
+// The first integer corresponds to the field number of the submessage set
+// inside FailureDetail's "oneof", which corresponds with the failure's
+// category.
+//
+// The second integer corresponds to the value of the enum at field number 1
+// within that submessage, which corresponds with the failure's subcategory.
+//
+// WARNING: This functionality is experimental and should not be relied on at
+// this time.
+// TODO(mschaller): remove experimental warning
message FailureDetail {
// A short human-readable message describing the failure, for debugging.
+ //
+ // This value is *not* intended to be used algorithmically.
string message = 1;
+
+ // Reserved for future generally applicable values. Any of these may be set.
+ reserved 2 to 100;
+
+ oneof category {
+ Interrupted interrupted = 101;
+ }
+}
+
+message Interrupted {
+ enum InterruptedCode {
+ // Interrupted at an unspecified time.
+ UNSPECIFIED = 0 [(metadata) = { exit_code: 8 }];
+ // TODO(mschaller): Add subcategories that specify what was happening at the
+ // time of interruption.
+ }
+
+ InterruptedCode code = 1;
}
diff --git a/src/test/java/com/google/devtools/build/lib/BUILD b/src/test/java/com/google/devtools/build/lib/BUILD
index bbe241f..97365d9 100644
--- a/src/test/java/com/google/devtools/build/lib/BUILD
+++ b/src/test/java/com/google/devtools/build/lib/BUILD
@@ -1075,6 +1075,7 @@
"//src/main/java/com/google/devtools/build/lib/vfs/inmemoryfs",
"//src/main/protobuf:command_server_java_grpc",
"//src/main/protobuf:command_server_java_proto",
+ "//src/main/protobuf:failure_details_java_proto",
"//src/main/protobuf:invocation_policy_java_proto",
"//third_party:jsr305",
"//third_party:mockito",
diff --git a/src/test/java/com/google/devtools/build/lib/server/GrpcServerTest.java b/src/test/java/com/google/devtools/build/lib/server/GrpcServerTest.java
index d64c35d7..71d50f1 100644
--- a/src/test/java/com/google/devtools/build/lib/server/GrpcServerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/server/GrpcServerTest.java
@@ -25,6 +25,7 @@
import com.google.devtools.build.lib.server.CommandProtos.RunRequest;
import com.google.devtools.build.lib.server.CommandProtos.RunResponse;
import com.google.devtools.build.lib.server.CommandServerGrpc.CommandServerStub;
+import com.google.devtools.build.lib.server.FailureDetails.Interrupted.InterruptedCode;
import com.google.devtools.build.lib.server.GrpcServerImpl.BlockingStreamObserver;
import com.google.devtools.build.lib.testutil.Suite;
import com.google.devtools.build.lib.testutil.TestSpec;
@@ -155,6 +156,7 @@
assertThat(responses.get(0).getCookie()).isNotEmpty();
assertThat(responses.get(1).getFinished()).isTrue();
assertThat(responses.get(1).getExitCode()).isEqualTo(0);
+ assertThat(responses.get(1).hasFailureDetail()).isFalse();
}
@Test
@@ -261,6 +263,7 @@
}
assertThat(responses.get(11).getFinished()).isTrue();
assertThat(responses.get(11).getExitCode()).isEqualTo(0);
+ assertThat(responses.get(11).hasFailureDetail()).isFalse();
}
@Test
@@ -321,7 +324,6 @@
@Test
public void testCancel() throws Exception {
- CountDownLatch done = new CountDownLatch(1);
CommandDispatcher dispatcher =
new CommandDispatcher() {
@Override
@@ -332,30 +334,34 @@
LockingMode lockingMode,
String clientDescription,
long firstContactTimeMillis,
- Optional<List<Pair<String, String>>> startupOptionsTaggedWithBazelRc) {
+ Optional<List<Pair<String, String>>> startupOptionsTaggedWithBazelRc)
+ throws InterruptedException {
synchronized (this) {
- try {
- this.wait();
- } catch (InterruptedException expected) {
- // Expected
- }
+ this.wait();
}
- done.countDown();
- return BlazeCommandResult.exitCode(ExitCode.INTERRUPTED);
+ // Interruption expected before this is reached.
+ throw new IllegalStateException();
}
};
createServer(dispatcher);
AtomicReference<String> commandId = new AtomicReference<>();
- CountDownLatch inResponse = new CountDownLatch(1);
+ CountDownLatch gotCommandId = new CountDownLatch(1);
+ AtomicReference<RunResponse> secondResponse = new AtomicReference<>();
+ CountDownLatch gotSecondResponse = new CountDownLatch(1);
CommandServerStub stub = CommandServerGrpc.newStub(channel);
stub.run(
createRequest("Foo"),
new StreamObserver<RunResponse>() {
@Override
public void onNext(RunResponse value) {
- commandId.set(value.getCommandId());
- inResponse.countDown();
+ String previousCommandId = commandId.getAndSet(value.getCommandId());
+ if (previousCommandId == null) {
+ gotCommandId.countDown();
+ } else {
+ secondResponse.set(value);
+ gotSecondResponse.countDown();
+ }
}
@Override
@@ -365,7 +371,7 @@
public void onCompleted() {}
});
// Wait until we've got the command id.
- inResponse.await();
+ gotCommandId.await();
CountDownLatch cancelRequestComplete = new CountDownLatch(1);
CancelRequest cancelRequest =
@@ -385,9 +391,16 @@
}
});
cancelRequestComplete.await();
- done.await();
+ gotSecondResponse.await();
server.shutdown();
server.awaitTermination();
+
+ assertThat(secondResponse.get().getFinished()).isTrue();
+ assertThat(secondResponse.get().getExitCode()).isEqualTo(8);
+ assertThat(secondResponse.get().hasFailureDetail()).isTrue();
+ assertThat(secondResponse.get().getFailureDetail().hasInterrupted()).isTrue();
+ assertThat(secondResponse.get().getFailureDetail().getInterrupted().getCode())
+ .isEqualTo(InterruptedCode.UNSPECIFIED);
}
@Test
