From 94cd33214c1e532f97b230d1c292ebed2a41c279 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 14 Aug 2024 02:32:39 -0700 Subject: [PATCH] Avoid pointless exception wrapping. 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 --- .../build/lib/worker/WorkerSpawnRunner.java | 19 ++------------ .../lib/worker/WorkerSpawnRunnerTest.java | 26 +++++++------------ 2 files changed, 11 insertions(+), 34 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java index 948f4d375da82a..a88548fd31d1d2 100644 --- a/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java +++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerSpawnRunner.java @@ -372,7 +372,7 @@ WorkResponse execInWorker( List flagFiles, InputMetadataProvider inputFileCache, SpawnMetrics.Builder spawnMetrics) - throws InterruptedException, ExecException { + throws ExecException, ForbiddenActionInputException, IOException, InterruptedException { WorkerOwner workerOwner = null; WorkResponse response; WorkRequest request; @@ -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()); @@ -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() diff --git a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java index 9a70b84886bf40..7c6a4adb5b1792 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java +++ b/src/test/java/com/google/devtools/build/lib/worker/WorkerSpawnRunnerTest.java @@ -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; @@ -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() @@ -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"); @@ -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"); @@ -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"); @@ -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; @@ -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; @@ -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); @@ -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"); @@ -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 =