Clean up nullability in the CommandEnvironment#pendingException field's value.
Distinguish between pendingException being unset, being set to a dummy value by #precompleteCommand, and being set to a not-dummy value BlazeModuleEnvironment#exit. Commit 54c9f1 inadvertently got rid of this distinction, since it disallowed the AbruptExitException ctor taking in a null ExitCode.
This distinction is potentially relevant for CommandEnvironment#getPendingExitCode. In this change here, I made that method private and cleaned up its usage.
The distinction was added pretty early in the history of Bazel in commit 6e5e8f. It didn't actually matter in the codebase back then, and it still doesn't now, because BuildTool#processRequest is called *before* CommandEnvironment#precompleteCommand (see the consecutive lines in BlazeCommandDispatcher...), but let's not make the code brittle in the face of future changes. I also wanted to save future code readers time; I spent ~10 mins convincing myself that the code was "correct" today.
RELNOTES: None
PiperOrigin-RevId: 303968383
diff --git a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
index 8a693ab..79913df 100644
--- a/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
+++ b/src/main/java/com/google/devtools/build/lib/buildtool/BuildTool.java
@@ -302,16 +302,17 @@
} catch (InterruptedException e) {
// We may have been interrupted by an error, or the user's interruption may have raced with
// an error, so check to see if we should report that error code instead.
- ExitCode environmentPendingExitCode = env.getPendingExitCode();
- if (environmentPendingExitCode == null) {
+ AbruptExitException environmentPendingAbruptExitException = env.getPendingException();
+ if (environmentPendingAbruptExitException == null) {
detailedExitCode = DetailedExitCode.justExitCode(ExitCode.INTERRUPTED);
env.getReporter().handle(Event.error("build interrupted"));
env.getEventBus().post(new BuildInterruptedEvent());
} else {
// Report the exception from the environment - the exception we're handling here is just an
// interruption.
- detailedExitCode = DetailedExitCode.justExitCode(environmentPendingExitCode);
- reportExceptionError(env.getPendingException());
+ detailedExitCode =
+ DetailedExitCode.justExitCode(environmentPendingAbruptExitException.getExitCode());
+ reportExceptionError(environmentPendingAbruptExitException);
result.setCatastrophe();
}
} catch (TargetParsingException | LoadingFailedException | ViewCreationFailedException e) {
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
index 50e6f9c..07a31ee 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/CommandEnvironment.java
@@ -54,6 +54,7 @@
import java.util.HashSet;
import java.util.List;
import java.util.Map;
+import java.util.Optional;
import java.util.Set;
import java.util.TreeMap;
import java.util.TreeSet;
@@ -96,7 +97,14 @@
private Path workingDirectory;
private String workspaceName;
private boolean haveSetupPackageCache = false;
- private AtomicReference<AbruptExitException> pendingException = new AtomicReference<>();
+
+ // This AtomicReference is set to:
+ // - null, if neither BlazeModuleEnvironment#exit nor #precompleteCommand have been called
+ // - Optional.of(e), if BlazeModuleEnvironment#exit has been called with value e
+ // - Optional.empty(), if #precompleteCommand was called before any call to
+ // BlazeModuleEnvironment#exit
+ private final AtomicReference<Optional<AbruptExitException>> pendingException =
+ new AtomicReference<>();
private final Object fileCacheLock = new Object();
@@ -117,7 +125,7 @@
public void exit(AbruptExitException exception) {
Preconditions.checkNotNull(exception);
Preconditions.checkNotNull(exception.getExitCode());
- if (pendingException.compareAndSet(null, exception)) {
+ if (pendingException.compareAndSet(null, Optional.of(exception))) {
// There was no exception, so we're the first one to ask for an exit. Interrupt the command.
commandThread.interrupt();
}
@@ -523,7 +531,7 @@
private ExitCode finalizeExitCode() {
// Set the pending exception so that further calls to exit(AbruptExitException) don't lead to
// unwanted thread interrupts.
- if (pendingException.compareAndSet(null, new AbruptExitException("", ExitCode.RESERVED))) {
+ if (pendingException.compareAndSet(null, Optional.empty())) {
return null;
}
if (Thread.currentThread() == commandThread) {
@@ -545,11 +553,9 @@
return finalizeExitCode();
}
- /**
- * Returns the current exit code requested by modules, or null if no exit has been requested.
- */
+ /** Returns the current exit code requested by modules, or null if no exit has been requested. */
@Nullable
- public ExitCode getPendingExitCode() {
+ private ExitCode getPendingExitCode() {
AbruptExitException exception = getPendingException();
return exception == null ? null : exception.getExitCode();
}
@@ -559,8 +565,10 @@
*
* <p>Prefer getPendingExitCode or throwPendingException where appropriate.
*/
+ @Nullable
public AbruptExitException getPendingException() {
- return pendingException.get();
+ Optional<AbruptExitException> abruptExitExceptionMaybe = pendingException.get();
+ return abruptExitExceptionMaybe == null ? null : abruptExitExceptionMaybe.orElse(null);
}
/**
