Refactor (Simple)SpawnResult to reduce invalid representations
This moves status-related helper method implementations from the concrete
implementation to default methods on the interface, making their
semantics clearer, and encouraging other implementations of the interface
to behave the same.
This also strengthens status/exitCode assertions in SpawnResult.Builder's
build method. This makes misuse less likely.
RELNOTES: None.
PiperOrigin-RevId: 296071684
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
index 8d09f96..6054f65 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
@@ -28,6 +28,9 @@
/** The result of a {@link Spawn}'s execution. */
public interface SpawnResult {
+
+ int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/ 128 + /*SIGALRM=*/ 14;
+
/** The status of the attempted Spawn execution. */
enum Status {
/** Subprocess executed successfully, and returned a zero exit code. */
@@ -91,17 +94,25 @@
}
/**
- * Returns whether the spawn was actually run, regardless of the exit code. I.e., returns
- * {@code true} if {@link #status} is any of {@link Status#SUCCESS}, {@link Status#NON_ZERO_EXIT},
- * {@link Status#TIMEOUT} or {@link Status#OUT_OF_MEMORY}.
+ * Returns whether the spawn was actually run, regardless of the exit code. I.e., returns {@code
+ * true} if {@link #status} is any of {@link Status#SUCCESS}, {@link Status#NON_ZERO_EXIT}, {@link
+ * Status#TIMEOUT} or {@link Status#OUT_OF_MEMORY}.
*
* <p>Returns false if there were errors that prevented the spawn from being run, such as network
* errors, missing local files, errors setting up sandboxing, etc.
*/
- boolean setupSuccess();
+ default boolean setupSuccess() {
+ Status status = status();
+ return status == Status.SUCCESS
+ || status == Status.NON_ZERO_EXIT
+ || status == Status.TIMEOUT
+ || status == Status.OUT_OF_MEMORY;
+ }
/** Returns true if the status was {@link Status#EXECUTION_FAILED_CATASTROPHICALLY}. */
- boolean isCatastrophe();
+ default boolean isCatastrophe() {
+ return status() == Status.EXECUTION_FAILED_CATASTROPHICALLY;
+ }
/** Returns the status of the attempted Spawn execution. */
Status status();
@@ -116,7 +127,10 @@
*
* <p>Returns 128 + 14 (corresponding to the Unix signal SIGALRM) if {@link #status} returns
* {@link Status#TIMEOUT}.
+ *
+ * <p>Otherwise, the returned value is not meaningful.
*/
+ // TODO(mschaller): clean up all uses of this method when {@code !this.setupSuccess()}
int exitCode();
/**
@@ -257,19 +271,6 @@
}
@Override
- public boolean setupSuccess() {
- return status == Status.SUCCESS
- || status == Status.NON_ZERO_EXIT
- || status == Status.TIMEOUT
- || status == Status.OUT_OF_MEMORY;
- }
-
- @Override
- public boolean isCatastrophe() {
- return status == Status.EXECUTION_FAILED_CATASTROPHICALLY;
- }
-
- @Override
public int exitCode() {
return exitCode;
}
@@ -416,10 +417,9 @@
Preconditions.checkArgument(!runnerName.isEmpty());
if (status == Status.SUCCESS) {
Preconditions.checkArgument(exitCode == 0);
- }
- if (status == Status.NON_ZERO_EXIT
- || status == Status.TIMEOUT
- || status == Status.OUT_OF_MEMORY) {
+ } else if (status == Status.TIMEOUT) {
+ Preconditions.checkArgument(exitCode == POSIX_TIMEOUT_EXIT_CODE);
+ } else if (status == Status.NON_ZERO_EXIT || status == Status.OUT_OF_MEMORY) {
Preconditions.checkArgument(exitCode != 0);
}
return new SimpleSpawnResult(this);
diff --git a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
index 3916de0..67e58c5 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/local/LocalSpawnRunner.java
@@ -38,7 +38,6 @@
import com.google.devtools.build.lib.exec.BinTools;
import com.google.devtools.build.lib.exec.RunfilesTreeUpdater;
import com.google.devtools.build.lib.exec.SpawnRunner;
-import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
@@ -75,7 +74,6 @@
private static final Joiner SPACE_JOINER = Joiner.on(' ');
private static final String UNHANDLED_EXCEPTION_MSG = "Unhandled exception running a local spawn";
private static final int LOCAL_EXEC_ERROR = -1;
- private static final int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/ 128 + /*SIGALRM=*/ 14;
private static final Logger logger = Logger.getLogger(LocalSpawnRunner.class.getName());
@@ -416,7 +414,8 @@
boolean wasTimeout =
terminationStatus.timedOut()
|| (useProcessWrapper && wasTimeout(context.getTimeout(), wallTime));
- int exitCode = wasTimeout ? POSIX_TIMEOUT_EXIT_CODE : terminationStatus.getRawExitCode();
+ int exitCode =
+ wasTimeout ? SpawnResult.POSIX_TIMEOUT_EXIT_CODE : terminationStatus.getRawExitCode();
Status status =
wasTimeout ? Status.TIMEOUT : (exitCode == 0 ? Status.SUCCESS : Status.NON_ZERO_EXIT);
SpawnResult.Builder spawnResultBuilder =
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 b4337be..e6c4e0e 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
@@ -100,8 +100,6 @@
@ThreadSafe
public class RemoteSpawnRunner implements SpawnRunner {
- private static final int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/ 128 + /*SIGALRM=*/ 14;
-
private static final String VIOLATION_TYPE_MISSING = "MISSING";
private static boolean retriableExecErrors(Exception e) {
@@ -460,7 +458,7 @@
return new SpawnResult.Builder()
.setRunnerName(getName())
.setStatus(Status.TIMEOUT)
- .setExitCode(POSIX_TIMEOUT_EXIT_CODE)
+ .setExitCode(SpawnResult.POSIX_TIMEOUT_EXIT_CODE)
.build();
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
index 9e39874..18e9eb6 100644
--- a/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/sandbox/AbstractSandboxSpawnRunner.java
@@ -51,7 +51,6 @@
/** Abstract common ancestor for sandbox spawn runners implementing the common parts. */
abstract class AbstractSandboxSpawnRunner implements SpawnRunner {
private static final int LOCAL_EXEC_ERROR = -1;
- private static final int POSIX_TIMEOUT_EXIT_CODE = /*SIGNAL_BASE=*/128 + /*SIGALRM=*/14;
private static final String SANDBOX_DEBUG_SUGGESTION =
"\n\nUse --sandbox_debug to see verbose messages from the sandbox";
@@ -193,7 +192,8 @@
boolean wasTimeout =
(useSubprocessTimeout && terminationStatus.timedOut())
|| (!useSubprocessTimeout && wasTimeout(timeout, wallTime));
- int exitCode = wasTimeout ? POSIX_TIMEOUT_EXIT_CODE : terminationStatus.getRawExitCode();
+ int exitCode =
+ wasTimeout ? SpawnResult.POSIX_TIMEOUT_EXIT_CODE : terminationStatus.getRawExitCode();
Status status =
wasTimeout
? Status.TIMEOUT
diff --git a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
index 59de66f..2e9b0ff 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/SpawnResultTest.java
@@ -38,7 +38,7 @@
new SpawnResult.Builder()
.setStatus(SpawnResult.Status.TIMEOUT)
.setWallTime(Duration.ofSeconds(5))
- .setExitCode(1)
+ .setExitCode(SpawnResult.POSIX_TIMEOUT_EXIT_CODE)
.setRunnerName("test")
.build();
assertThat(r.getDetailMessage("", "", false, false, false))
@@ -50,7 +50,7 @@
SpawnResult r =
new SpawnResult.Builder()
.setStatus(SpawnResult.Status.TIMEOUT)
- .setExitCode(1)
+ .setExitCode(SpawnResult.POSIX_TIMEOUT_EXIT_CODE)
.setRunnerName("test")
.build();
assertThat(r.getDetailMessage("", "", false, false, false))
