Skip to content

Commit

Permalink
Avoid pointless exception wrapping.
Browse files Browse the repository at this point in the history
SpawnRunner#exec is already allowed to return a ForbiddenActionInputException or IOException, which are converted into an ExecException in AbstractSpawnStrategy#exec. The wrapping provides no useful context, as this error occurs during input prefetching, which is independent from the spawn runner.

(Note: the PREFETCH_FAILURE code will be replaced with a generic EXEC_IO_EXCEPTION, but the former is only used by the WorkerSpawnRunner, which is weird anyway. If we cared sufficiently about the distinction, it would make more sense to throw an EnvironmentalExecException from prefetchInputsAndWait, regardless of the runner being used.)

PiperOrigin-RevId: 662839172
Change-Id: Ic04f8907931a60bd6cc9130340c4a6ef4d3f9c80
  • Loading branch information
tjgq authored and copybara-github committed Aug 14, 2024
1 parent 6052ad6 commit 94cd332
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 34 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ WorkResponse execInWorker(
List<String> flagFiles,
InputMetadataProvider inputFileCache,
SpawnMetrics.Builder spawnMetrics)
throws InterruptedException, ExecException {
throws ExecException, ForbiddenActionInputException, IOException, InterruptedException {
WorkerOwner workerOwner = null;
WorkResponse response;
WorkRequest request;
Expand All @@ -385,16 +385,7 @@ WorkResponse execInWorker(

try (SilentCloseable c =
Profiler.instance().profile(ProfilerTask.WORKER_SETUP, "Preparing inputs")) {
try {
context.prefetchInputsAndWait();
} catch (IOException e) {
restoreInterrupt(e);
String message = "IOException while prefetching for worker:";
throw createUserExecException(e, message, Code.PREFETCH_FAILURE);
} catch (ForbiddenActionInputException e) {
throw createUserExecException(
e, "Forbidden input found while prefetching for worker:", Code.FORBIDDEN_INPUT);
}
context.prefetchInputsAndWait();
}
Duration setupInputsTime = setupInputsStopwatch.elapsed();
spawnMetrics.setSetupTimeInMs((int) setupInputsTime.toMillis());
Expand Down Expand Up @@ -705,12 +696,6 @@ private static UserExecException createUserExecException(
ErrorMessage.builder().message(message).exception(e).build().toString(), detailedCode);
}

private static UserExecException createUserExecException(
ForbiddenActionInputException e, String message, Code detailedCode) {
return createUserExecException(
ErrorMessage.builder().message(message).exception(e).build().toString(), detailedCode);
}

private static UserExecException createUserExecException(String message, Code detailedCode) {
return new UserExecException(
FailureDetail.newBuilder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.ExecException;
import com.google.devtools.build.lib.actions.ExecutionRequirements;
import com.google.devtools.build.lib.actions.ExecutionRequirements.WorkerProtocolFormat;
import com.google.devtools.build.lib.actions.InputMetadataProvider;
Expand Down Expand Up @@ -99,7 +98,7 @@ public class WorkerSpawnRunnerTest {
@Mock ResourceManager.ResourceHandle resourceHandle;

@Before
public void setUp() throws InterruptedException, IOException, ExecException {
public void setUp() throws Exception {
when(spawn.getInputFiles()).thenReturn(NestedSetBuilder.emptySet(Order.COMPILE_ORDER));
when(context.getArtifactExpander()).thenReturn(treeArtifact -> ImmutableSortedSet.of());
doNothing()
Expand All @@ -112,7 +111,7 @@ public void setUp() throws InterruptedException, IOException, ExecException {
}

@Test
public void testExecInWorker_happyPath() throws ExecException, InterruptedException, IOException {
public void testExecInWorker_happyPath() throws Exception {
WorkerSpawnRunner runner = createWorkerSpawnRunner(new WorkerOptions());
WorkerKey key = createWorkerKey(fs, "mnem", false);
Path logFile = fs.getPath("/worker.log");
Expand Down Expand Up @@ -141,8 +140,7 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept
}

@Test
public void testExecInWorker_virtualInputs_doesntQueryInputFileCache()
throws ExecException, InterruptedException, IOException {
public void testExecInWorker_virtualInputs_doesntQueryInputFileCache() throws Exception {
Path execRoot = fs.getPath("/execRoot");
Path workDir = execRoot.getRelative("workdir");

Expand Down Expand Up @@ -199,8 +197,7 @@ public void testExecInWorker_virtualInputs_doesntQueryInputFileCache()
}

@Test
public void testExecInWorker_finishesAsyncOnInterrupt()
throws InterruptedException, IOException, ExecException {
public void testExecInWorker_finishesAsyncOnInterrupt() throws Exception {
WorkerSpawnRunner runner = createWorkerSpawnRunner(new WorkerOptions());
WorkerKey key = createWorkerKey(fs, "mnem", false);
Path logFile = fs.getPath("/worker.log");
Expand Down Expand Up @@ -228,8 +225,7 @@ public void testExecInWorker_finishesAsyncOnInterrupt()
}

@Test
public void testExecInWorker_sendsCancelMessageOnInterrupt()
throws ExecException, InterruptedException, IOException {
public void testExecInWorker_sendsCancelMessageOnInterrupt() throws Exception {
WorkerOptions workerOptions = new WorkerOptions();
workerOptions.workerCancellation = true;
workerOptions.workerSandboxing = true;
Expand Down Expand Up @@ -280,8 +276,7 @@ public void testExecInWorker_sendsCancelMessageOnInterrupt()
}

@Test
public void testExecInWorker_unsandboxedDiesOnInterrupt()
throws InterruptedException, IOException, ExecException {
public void testExecInWorker_unsandboxedDiesOnInterrupt() throws Exception {
WorkerOptions workerOptions = new WorkerOptions();
workerOptions.workerCancellation = true;
workerOptions.workerSandboxing = false;
Expand Down Expand Up @@ -317,8 +312,7 @@ public void testExecInWorker_unsandboxedDiesOnInterrupt()
}

@Test
public void testExecInWorker_noMultiplexWithDynamic()
throws ExecException, InterruptedException, IOException {
public void testExecInWorker_noMultiplexWithDynamic() throws Exception {
WorkerOptions workerOptions = new WorkerOptions();
workerOptions.workerMultiplex = true;
WorkerSpawnRunner runner = createWorkerSpawnRunner(workerOptions);
Expand Down Expand Up @@ -407,8 +401,7 @@ public void testExecInWorker_throwsWithEmptyResponse() throws Exception {
}

@Test
public void testExpandArgument_expandsArgumentsRecursively()
throws IOException, InterruptedException {
public void testExpandArgument_expandsArgumentsRecursively() throws Exception {
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
FileSystemUtils.writeIsoLatin1(fs.getPath("/file"), "arg1\n@file2\nmulti arg\n");
FileSystemUtils.writeIsoLatin1(fs.getPath("/file2"), "arg2\narg3");
Expand All @@ -427,8 +420,7 @@ public void testExpandArgument_expandsArgumentsRecursively()
}

@Test
public void testExpandArgument_expandsOnlyProperArguments()
throws IOException, InterruptedException {
public void testExpandArgument_expandsOnlyProperArguments() throws Exception {
WorkRequest.Builder requestBuilder = WorkRequest.newBuilder();
FileSystemUtils.writeIsoLatin1(fs.getPath("/file"), "arg1\n@@nonfile\n@foo//bar\narg2");
SandboxInputs inputs =
Expand Down

0 comments on commit 94cd332

Please sign in to comment.