Add support for wrapping system streams in WorkRequestHandler #14201

Bencodes · 2021-11-01T03:30:44Z

There are often places where persistent workers need to swap out the standard system streams to avoid tools poisoning the worker communication streams by writing logs/exceptions to it.

This pull request extracts that pattern into an optional WorkerIO wrapper can be used to swap in and out the standard streams without the added boilerplate.

Bencodes · 2021-11-17T17:29:08Z

@larsrc-google mind taking a look at this?

Bencodes · 2021-11-17T19:54:55Z

src/main/java/com/google/devtools/build/lib/worker/WorkRequestHandler.java

+
+    private final InputStream originalInputStream;
+    private final PrintStream originalOutputStream;
+    private final PrintStream originalErrorStream;


Wrapping System.err isn't actually be necessary for a wrapper worker io.

Correct. Also System.in is already handled properly. Only System.out needs to be wrapped.

Also System.in is already handled properly. Only System.out needs to be wrapped.

The motivation behind wrapping all of the system streams in a WorkerIO class is that there is a clear and consistent place to access the root system streams while setting up the work request handler. For System.in and System.err we can just return the original streams when calling getOriginalInputStream.

Upon further reflection, what you do makes sense, but not in the way you use it in the example. Instead of wrapping the whole WorkRequestHandler, wrapping the inside of the WorkRequestCallback would be clearer. You wouldn't even need to have the accessors to the original streams. That prevents the tool from attempting to read from stdin and corrupting the communication.

That change would also mean that the WorkerIO doesn't need to reset the buffer, as it'll be thrown away once a request is done. Plus in case processRequest in your example throws an exception the WorkerIO will be closed properly.

A definite benefit of either approach is that stdout and stderr get merged into a single stream, that will be easier to understand.

wrapping the inside of the WorkRequestCallback would be clearer

By this do you mean calling capture from within the implementation of WorkRequestHandler, or in the lambda that gets passed into the WorkRequestHandler? I like idea of this being hidden inside the WorkRequestCallback because we hide WorkerIO#readCapturedAsUtf8String entirely which is kind of an awkward API to need to expose.

That change would also mean that the WorkerIO doesn't need to reset the

Would each work request call WorkerIO#capture swapping in it's own buffer?

A definite benefit of either approach is that stdout and stderr get merged into a single stream, that will be easier to understand.

Wrapping the stderr could be a bit error-prone because if there's ever an unrecoverable error that causes the worker to not send a work response, you just won't see an exception come out of the worker.

How does your current solution deal with multiplex workers?

@larsrc-google I took another pass at building this into the WorkRequestHandler so that it's a bit more transparent, and with the multiplex worker cases in mind. The new approach uses the existing WorkerIO implementation, but wraps the system streams when you call WorkRequestHandler#processRequests.

The WorkerIO instance gets passed down into respondToRequest so that there is a single source of truth for the wrapped streams, and so that we don't have to deal with multiple WorkerIO instances being created inside of respondToRequest which doesn't really make sense in a multiplex-world where respondToRequest may get called concurrently.

Oh, I've been so focused on the implementation details that I forgot the larger problem: Redirecting the stderr messages into the output field of the response changes semantics. The output field is for shorter messages to be displayed to the user, stderr goes into the logs. See https://docs.bazel.build/versions/main/creating-workers.html#work-responses. It's known that multiplex workers only have one log between them, but the messages shown to the user should not interleave.

And of course, since the multiplex threads still share one System.err instance regardless of what we set, we can't possibly disentangle that. But we can still fix the main issue of stdout getting corrupted by setting it to the same stream as stderr.

Just to make sure i'm understanding next steps here - You'd prefer that we avoid capturing the tool output streams and instead just swap the System.err in for System.out to not pollute the work response output?

Yes. Having System.err and System.out go to the same place (the logs) is good. Changing the work response output would cause a lot of noise.

Pushed the changes for swapping System.err in for System.out and updated the tests.

larsrc-google · 2021-11-22T17:32:23Z

Sorry for not reacting earlier, need to update mail filters.

This is a good thing to do, something I have considered but not gotten around to yet. One question: Why an optional wrapper? Why not just make it part of WorkRequestHandler? Can you imagine a case where a user of WorkRequestHandler would not want to redirect stdout?

Bencodes · 2021-11-22T19:25:58Z

Why an optional wrapper? Why not just make it part of WorkRequestHandler?

No reason other than it felt easier to reason about merging this into Bazel as an isolated tool first, instead of making changes to WorkRequestHandler directly.

Can you imagine a case where a user of WorkRequestHandler would not want to redirect stdout?

I can't, although the java workers seem to be managing fine without wrapped system streams.

Bencodes · 2021-12-04T19:58:24Z

@larsrc-google is there a path forward for merging some form of this?

larsrc-google · 2022-01-17T08:29:42Z

src/main/java/com/google/devtools/build/lib/worker/WorkRequestHandler.java

+
+      // Redirect System.out to System.err
+      System.setOut(originalErrorStream);
+      System.setErr(originalErrorStream);


This looks a lot simpler. I don't see any reason to do anything to System.err, though.

larsrc-google · 2022-01-24T10:50:50Z

src/main/java/com/google/devtools/build/lib/worker/WorkRequestHandler.java

@@ -311,6 +315,8 @@ public WorkRequestHandler build() {
   * returns. If {@code in} reaches EOF, it also returns.
   */
  public void processRequests() throws IOException {
+    WorkerIO workerIO = WorkerIO.redirectSystemStreams();


Looking at #14623, you're not the only one with this idea. You might be right in making it optional, as some workers do their own handling. Fortunately that could easily be done by adding a method to the builder.

nkoroste · 2022-05-03T19:14:59Z

Any updates on this?

larsrc-google · 2022-08-25T15:28:34Z

Sorry for the silence, I was out sick for quite a while. I believe being optional is not as big a deal as I indicated, so if you can fix the conflicts, we should be able to merge this.

Bencodes · 2022-08-25T17:01:39Z

@larsrc-google I have this a rebase. Please give it another once over to make sure that the conflicts got resolved correctly against the new changes that were in master.

There are often [places](https://github.com/bazelbuild/bazel/blob/ea19c17075478092eb77580e6d3825d480126d3a/src/tools/android/java/com/google/devtools/build/android/ResourceProcessorBusyBox.java#L188) where persistent workers need to swap out the standard system streams to avoid tools poisoning the worker communication streams by writing logs/exceptions to it. This pull request extracts that pattern into an optional WorkerIO wrapper can be used to swap in and out the standard streams without the added boilerplate. Closes #14201. PiperOrigin-RevId: 498983983 Change-Id: Iefb956d38a5887d9e5bbf0821551eb0efa14fce9

brentleyjones · 2023-02-24T15:03:24Z

@bazel-io flag

brentleyjones · 2023-02-24T15:17:36Z

(Above was from a discussion with @larsrc-google: #17448 (comment))

ShreeM01 · 2023-02-24T16:13:15Z

@bazel-io fork 6.1.0

With the baked in `WorkerIO` added here #14201 we can remove the custom system stream wrapping and instead rely on `WorkRequestHandler` to manage that for us. Closes #17654. PiperOrigin-RevId: 559454874 Change-Id: I683ca6580bb7d7da3b7712e0033e1b903010de5b

This pull request provides a custom `DiagnosticsHandler` implementation that redirects info and warning messages to the original `System#err` stream so that the noisy D8 messages are written to the worker log file instead of directly to the console. In addition this PR also removes the custom system stream redirection, and instead uses the baked in `WorkerIO` implementation that automatically wraps the system stream for us that was added here #14201. #17647 Closes #17648. PiperOrigin-RevId: 560600558 Change-Id: I9d2d162b62fe5dfd00e8614ba73caf1cac5f02d7

google-cla bot added the cla: yes label Nov 1, 2021

oquenchil assigned larsrc-google Nov 2, 2021

oquenchil requested a review from larsrc-google November 2, 2021 14:37

Bencodes commented Nov 17, 2021

View reviewed changes

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch from 04ac34c to cc18fc2 Compare November 22, 2021 19:56

philwo force-pushed the master branch from 26cb401 to 168b89b Compare December 2, 2021 18:07

Bencodes mentioned this pull request Dec 14, 2021

Support multiplex workers in ResourceProcessorBusyBox #14428

Closed

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch from 901c960 to 2856b3b Compare January 10, 2022 04:33

Bencodes requested review from ahumesky, aiuto, brandjon, floriographygoth, gregestren, jin, kevin1e100, lberki, oquenchil, tetromino and a team as code owners January 10, 2022 04:33

google-cla bot added cla: no and removed cla: yes labels Jan 10, 2022

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch from 2856b3b to 008280a Compare January 10, 2022 04:39

google-cla bot added cla: yes and removed cla: no labels Jan 10, 2022

lberki removed request for lberki and ahumesky January 10, 2022 08:20

lberki removed request for kevin1e100, oquenchil, tetromino and a team January 10, 2022 08:21

larsrc-google reviewed Jan 17, 2022

View reviewed changes

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch 2 times, most recently from 9a11aa4 to 911ea81 Compare January 21, 2022 22:34

larsrc-google reviewed Jan 24, 2022

View reviewed changes

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch from 911ea81 to fdc6e07 Compare January 24, 2022 23:42

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch from 3da7148 to 6e5c7cb Compare February 6, 2022 17:34

Add support for wrapping system streams in WorkRequestHandler

2d3689f

Bencodes force-pushed the add-support-for-wrapping-system-streams-in-workrequesthandler branch from 6e5c7cb to 2d3689f Compare August 25, 2022 15:56

copybara-service bot closed this in e05345d Jan 2, 2023

larsrc-google mentioned this pull request Feb 14, 2023

Writing to standard out from a java annotation processor produces incomprehensible error message #17448

Closed

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 24, 2023

bazel-io mentioned this pull request Feb 24, 2023

[6.1.0] Add support for wrapping system streams in WorkRequestHandler #17584

Closed

bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Feb 24, 2023

ShreeM01 mentioned this pull request Feb 24, 2023

[6.1.0]Add support for wrapping system streams in WorkRequestHandler #17593

Closed

Bencodes deleted the add-support-for-wrapping-system-streams-in-workrequesthandler branch March 2, 2023 01:32

This was referenced Mar 2, 2023

Avoid writing D8 info and error messages directly to the Bazel console #17648

Closed

Remove custom system stream wrapping from ResourceProcessorBusyBox #17654

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for wrapping system streams in WorkRequestHandler #14201

Add support for wrapping system streams in WorkRequestHandler #14201

Bencodes commented Nov 1, 2021 •

edited

Loading

Bencodes commented Nov 17, 2021

Bencodes Nov 17, 2021

larsrc-google Nov 30, 2021

Bencodes Dec 4, 2021

larsrc-google Dec 14, 2021

Bencodes Dec 20, 2021

Bencodes Jan 10, 2022

larsrc-google Jan 10, 2022

Bencodes Jan 10, 2022

larsrc-google Jan 12, 2022

Bencodes Jan 13, 2022

larsrc-google commented Nov 22, 2021

Bencodes commented Nov 22, 2021

Bencodes commented Dec 4, 2021

larsrc-google Jan 17, 2022

Bencodes Jan 17, 2022

larsrc-google Jan 24, 2022

nkoroste commented May 3, 2022

larsrc-google commented Aug 25, 2022

Bencodes commented Aug 25, 2022

brentleyjones commented Feb 24, 2023

brentleyjones commented Feb 24, 2023 •

edited

Loading

ShreeM01 commented Feb 24, 2023

Add support for wrapping system streams in WorkRequestHandler #14201

Add support for wrapping system streams in WorkRequestHandler #14201

Conversation

Bencodes commented Nov 1, 2021 • edited Loading

Bencodes commented Nov 17, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larsrc-google commented Nov 22, 2021

Bencodes commented Nov 22, 2021

Bencodes commented Dec 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nkoroste commented May 3, 2022

larsrc-google commented Aug 25, 2022

Bencodes commented Aug 25, 2022

brentleyjones commented Feb 24, 2023

brentleyjones commented Feb 24, 2023 • edited Loading

ShreeM01 commented Feb 24, 2023

Bencodes commented Nov 1, 2021 •

edited

Loading

brentleyjones commented Feb 24, 2023 •

edited

Loading