From 0881c80d29acecdfbb58c49156f805e8c50db117 Mon Sep 17 00:00:00 2001 From: larsrc Date: Sun, 22 Nov 2020 06:16:38 -0800 Subject: [PATCH] Don't set requestId on non-multiplex requests. RELNOTES: n/a PiperOrigin-RevId: 343728512 --- .../build/lib/worker/WorkerSpawnRunner.java | 10 ++++-- .../build/lib/worker/ExampleWorker.java | 10 ++++++ .../lib/worker/ExampleWorkerOptions.java | 19 +++++++---- .../lib/worker/WorkerSpawnRunnerTest.java | 9 +++-- .../shell/integration/bazel_worker_test.sh | 33 +++++++++++++++++++ 5 files changed, 67 insertions(+), 14 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 386bf809a8c23c..7be36ea8a655d3 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 @@ -293,7 +293,8 @@ private WorkRequest createWorkRequest( Spawn spawn, SpawnExecutionContext context, List flagfiles, - MetadataProvider inputFileCache) + MetadataProvider inputFileCache, + WorkerKey key) throws IOException { WorkRequest.Builder requestBuilder = WorkRequest.newBuilder(); for (String flagfile : flagfiles) { @@ -318,7 +319,10 @@ private WorkRequest createWorkRequest( .setDigest(digest) .build(); } - return requestBuilder.setRequestId(requestIdCounter.getAndIncrement()).build(); + if (key.getProxied()) { + requestBuilder.setRequestId(requestIdCounter.getAndIncrement()); + } + return requestBuilder.build(); } /** @@ -420,7 +424,7 @@ WorkResponse execInWorker( try { worker = workers.borrowObject(key); worker.setReporter(workerOptions.workerVerbose ? reporter : null); - request = createWorkRequest(spawn, context, flagFiles, inputFileCache); + request = createWorkRequest(spawn, context, flagFiles, inputFileCache, key); } catch (IOException e) { String message = "IOException while borrowing a worker from the pool:"; throw createUserExecException(e, message, Code.BORROW_FAILURE); diff --git a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java index 16fe7d9fdfb659..471218c060dbda 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java +++ b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorker.java @@ -55,6 +55,9 @@ public class ExampleWorker { // Keep state across multiple builds. static final LinkedHashMap inputs = new LinkedHashMap<>(); + // Contains the request currently being worked on. + private static WorkRequest currentRequest; + public static void main(String[] args) throws Exception { if (ImmutableSet.copyOf(args).contains("--persistent_worker")) { OptionsParser parser = @@ -92,6 +95,8 @@ private static void runPersistentWorker(ExampleWorkerOptions workerOptions) thro break; } + currentRequest = request; + inputs.clear(); for (Input input : request.getInputsList()) { inputs.put(input.getPath(), input.getDigest().toStringUtf8()); @@ -127,6 +132,7 @@ private static void runPersistentWorker(ExampleWorkerOptions workerOptions) thro } finally { System.setOut(originalStdOut); System.setErr(originalStdErr); + currentRequest = null; } if (workerOptions.exitDuring > 0 && workUnitCounter > workerOptions.exitDuring) { @@ -198,6 +204,10 @@ private static void processRequest(List args) throws Exception { } } + if (options.printRequests) { + outputs.add("REQUEST: " + currentRequest); + } + if (options.printEnv) { for (Map.Entry entry : System.getenv().entrySet()) { outputs.add(entry.getKey() + "=" + entry.getValue()); diff --git a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorkerOptions.java b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorkerOptions.java index 3e78481477279a..40d6faa5811b4a 100644 --- a/src/test/java/com/google/devtools/build/lib/worker/ExampleWorkerOptions.java +++ b/src/test/java/com/google/devtools/build/lib/worker/ExampleWorkerOptions.java @@ -66,12 +66,19 @@ public static class ExampleWorkOptions extends OptionsBase { public boolean writeCounter; @Option( - name = "print_inputs", - documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, - effectTags = {OptionEffectTag.NO_OP}, - defaultValue = "false", - help = "Writes a list of input files and their digests." - ) + name = "print_requests", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "false", + help = "Prints out all requests.") + public boolean printRequests; + + @Option( + name = "print_inputs", + documentationCategory = OptionDocumentationCategory.UNCATEGORIZED, + effectTags = {OptionEffectTag.NO_OP}, + defaultValue = "false", + help = "Writes a list of input files and their digests.") public boolean printInputs; @Option( 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 224c200609dae9..0b035a923ad465 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 @@ -110,9 +110,8 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept WorkerKey key = createWorkerKey(fs, "mnem", false); Path logFile = fs.getPath("/worker.log"); when(worker.getLogFile()).thenReturn(logFile); - when(worker.getResponse(1)) - .thenReturn( - WorkResponse.newBuilder().setExitCode(0).setOutput("out").setRequestId(1).build()); + when(worker.getResponse(0)) + .thenReturn(WorkResponse.newBuilder().setExitCode(0).setOutput("out").build()); WorkResponse response = runner.execInWorker( spawn, @@ -126,7 +125,7 @@ public void testExecInWorker_happyPath() throws ExecException, InterruptedExcept assertThat(response).isNotNull(); assertThat(response.getExitCode()).isEqualTo(0); - assertThat(response.getRequestId()).isEqualTo(1); + assertThat(response.getRequestId()).isEqualTo(0); assertThat(response.getOutput()).isEqualTo("out"); assertThat(logFile.exists()).isFalse(); } @@ -149,7 +148,7 @@ private void assertRecordedResponsethrowsException(String recordedResponse, Stri WorkerKey key = createWorkerKey(fs, "mnem", false); Path logFile = fs.getPath("/worker.log"); when(worker.getLogFile()).thenReturn(logFile); - when(worker.getResponse(1)).thenThrow(new IOException("Bad protobuf")); + when(worker.getResponse(0)).thenThrow(new IOException("Bad protobuf")); when(worker.getRecordingStreamMessage()).thenReturn(recordedResponse); String workerLog = "Log from worker\n"; FileSystemUtils.writeIsoLatin1(logFile, workerLog); diff --git a/src/test/shell/integration/bazel_worker_test.sh b/src/test/shell/integration/bazel_worker_test.sh index f1dc2e224e06cc..438fb992259909 100755 --- a/src/test/shell/integration/bazel_worker_test.sh +++ b/src/test/shell/integration/bazel_worker_test.sh @@ -206,6 +206,39 @@ EOF assert_equals "HELLO WORLD" "$(cat $BINS/hello_world_uppercase.out)" } +function test_worker_requests() { + prepare_example_worker + cat >>BUILD < $TEST_log \ + || fail "build failed" + assert_contains "hello world" "$BINS/hello_world.out" + assert_contains "arguments: \"hello world\"" "$BINS/hello_world.out" + assert_contains "path:.*hello_world_worker_input" "$BINS/hello_world.out" + assert_not_contains "request_id" "$BINS/hello_world.out" + + bazel build :hello_world_uppercase &> $TEST_log \ + || fail "build failed" + assert_contains "HELLO WORLD" "$BINS/hello_world_uppercase.out" + assert_contains "arguments: \"hello world\"" "$BINS/hello_world_uppercase.out" + assert_contains "path:.*hello_world_uppercase_worker_input" "$BINS/hello_world_uppercase.out" + assert_not_contains "request_id" "$BINS/hello_world_uppercase.out" +} + function test_shared_worker() { prepare_example_worker cat >>BUILD <