clean up workers #1005

andyscott · 2020-02-19T21:36:08Z

Description

Simplifies and cleans up the workers and the shared worker infrastructure.

First of all, I believe the diff makes the changes look complicated when it's mostly just refactoring and some signature changes.

A hopefully exhaustive list of changes:

Updates to the worker base:

GenericWorker and Processor are collapsed into a single Worker base.
Basic tests are added for the new Worker base
All stdout/stderr/stdin wrapping is moved to the worker base. Previously some of this logic was handled by each worker implementation.
The appendToString and merge methods were only used by the Scalac worker implementation, so they were moved out of the worker base.
Exit trap handling was added. This makes it safer to wrap other libraries that may attempt to exit and inadvertently shut down a worker process.
The main interface was changed from passing a java.util.List<String> to a String array to make Scala and Java both feel like first class citizens when writing worker implementations.

Updates to worker implementations:

ScalacProcessor and ScalaCInvoker are collapsed into ScalacWorker. Scalac worker specific methods were moved out of the shared worker base and into the Scalac worker.
ScalafmtWorker and ScalaformatRunner are collapsed into ScalafmtWorker. Exception handling is now managed by the worker base.
ScroogeWorker and ScroogeGenerator are collapsed into ScroogeWorker. Exception handling is now managed by the worker base.
ScalaPBWorker and ScalaPGBenerator are collapsed into ScalaPBWorker. Exception handling and stdout/stderr setup are now managed by the worker base.
JacocoInstrumenter was mostly unchanged as the worker implementation already aligned well with the simplified worker base.

Motivation

This isn't critical or necessary work, but it does provide a few niceties:

Scala and Java are now both first class citizens for writing workers (due to use of String arrays instead of Java lists).
Previously each worker leveraged the worker base a bit differently. Now it's all the same, so there's no ambiguity about which pattern to follow when writing a new worker implementation.
Cleanup! Less code and fewer classes if you exclude the introduction of new of tests. Shared code should go in the shared placed, and worker specific code belongs with that worker.

I know I said I would hold off on any cleanup. This is perhaps my only exception, as it's code split out from the existing PR #959 and leverages some learnings from when I implemented the Jacoco worker.

src/java/io/bazel/rulesscala/scalac/ScalacWorker.java

ittaiz · 2020-02-20T04:20:58Z

I’ll take a look, can you share a bit more info on what you changed in the design and why it’s easier?
There are a lot of details in the PR and from a quick glance I didn’t see yet why it’s easier

andyscott · 2020-02-20T18:20:21Z

Yep! I'll update the original message.

borkaehw

I feel it's simpler to merge GenericWorker and Processor, but I will defer to Andy for more explanation.

Just came to my mind. If we are refactoring (if it counts) worker, do we consider to rewrite it in Scala?

src/java/io/bazel/rulesscala/coverage/instrumenter/JacocoInstrumenter.java

…[\]|String[] args|'

andyscott · 2020-02-20T19:14:42Z

Just came to my mind. If we are refactoring (if it counts) worker, do we consider to rewrite it in Scala?

You'd need a way to bootstrap a Scala compiler to compile the worker base itself.

In higherkindness/rules_scala there was a bootstrap compiler backend that shelled out to scalac directly. We don't have that here-- and I'm not convinced that it's worth it-- so there has to be a worker base in Java.

borkaehw · 2020-02-20T19:19:15Z

You'd need a way to bootstrap a Scala compiler to compile the worker base itself.

It makes sense and thanks for the explanation.

ittaiz · 2020-02-21T13:57:40Z

Thanks Andy for the detailed description!
It definitely makes my work easier...

ittaiz

Thanks.
I added some comments and questions.
I hope I didn't miss any nuances in the move between the GenericWorker and the Worker because I found some changes and maybe missed others.
In tests we trust ;)

src/java/io/bazel/rulesscala/worker/Worker.java

src/java/io/bazel/rulesscala/worker/GenericWorker.java

src/java/io/bazel/rulesscala/worker/Worker.java

src/java/io/bazel/rulesscala/scalac/ScalacWorker.java

ittaiz · 2020-02-21T14:13:20Z

src/java/io/bazel/rulesscala/worker/Worker.java

+	    ByteArrayOutputStream outStream = new ByteArrayOutputStream();
+	    PrintStream out = new PrintStream(outStream);
+
+	    System.setIn(new ByteArrayInputStream(new byte[0]));


Why is this needed?

And also how does it work with you not calling System.in later on to get this input stream?

I'll add a comment, but basically: there is no stdin. So we attach stdin to an empty buffer and call it a day.

If I ever get around to finishing the standalone worker tests it would make this more obvious.

Comment added.

The idea is that Worker.Interface implementations shouldn't access stdin because that's the transport layer of the persistent worker itself? And so you "null it out" so they won't mess things up?
If so I'd love for the comment to reflect that because I didn't understand what "can't support" means

src/java/io/bazel/rulesscala/worker/GenericWorker.java

src/java/io/bazel/rulesscala/worker/Worker.java

src/java/io/bazel/rulesscala/worker/BUILD

src/java/io/bazel/rulesscala/worker/GenericWorker.java

borkaehw · 2020-03-05T19:48:05Z

Hi I am wondering what's the status of this PR? I am interested in moving forward on the discussion of #959.

ittaiz · 2020-03-05T20:00:23Z

I‘ve been somewhat occupied and had a few offline discussions with Andy where he said this work isn’t really blocking and can wait a bit. If this is blocking you I’ll push it up (I’ll try this weekend)

On Thu, 5 Mar 2020 at 21:48 Bor Kae Hwang ***@***.***> wrote: Hi I am wondering what's the status of this PR? I am interested in moving forward on the discussion of #959 <#959>. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1005?email_source=notifications&email_token=AAKQQF6RVGAT2WB24CZRSQDRF76YDA5CNFSM4KYCEV7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEN6UXCY#issuecomment-595413899>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF5JQR6FQR4XQ2V3S4LRF76YDANCNFSM4KYCEV7A> .

-- *Ittai Zeidman* 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>

borkaehw · 2020-03-05T20:08:17Z

Lucid is still on the way to migrate to bazelbuild/rules_scala, the major blocker now is the scala_test framework since we use specs2. #959 should fix it then we can start the migration process step by step.

I would say this PR is not blocking us but #959 is. But I suppose #959 is blocked by this PR.

ittaiz · 2020-03-05T21:29:56Z

Just to make sure- you’re not using specs2 junit runner, right?

borkaehw · 2020-03-06T17:49:30Z

I don't think so.

ittaiz · 2020-02-22T08:39:10Z

src/java/io/bazel/rulesscala/worker/Worker.java

+	    ByteArrayOutputStream outStream = new ByteArrayOutputStream();
+	    PrintStream out = new PrintStream(outStream);
+
+	    System.setIn(new ByteArrayInputStream(new byte[0]));


The idea is that Worker.Interface implementations shouldn't access stdin because that's the transport layer of the persistent worker itself? And so you "null it out" so they won't mess things up?
If so I'd love for the comment to reflect that because I didn't understand what "can't support" means

src/java/io/bazel/rulesscala/worker/WorkerTest.java

ittaiz · 2020-03-07T09:59:52Z

src/java/io/bazel/rulesscala/worker/Worker.java

+		    out.flush();
+		    outStream.reset();


I feel like I'm jerking your chain a bit but I was actually talking only about the gc line as that is what the old implementation did.
doing flush and reset in a finally block feels a bit adventurous :)

I actually think this is reasonable! This way we guarantee each new work request doesn't get any leftover output if a previous work request caused a crash.

borkaehw · 2020-03-16T16:47:04Z

@andyscott Friendly ping here, do you get a chance to take another look?

borkaehw · 2020-04-13T21:01:26Z

@andyscott Friendly ping here again (sorry for bothering). We would like to see this moving forward, we are also happy to help if you are busy. Thanks.

ittaiz · 2020-04-13T21:39:28Z

I was also thinking of picking this us given the remainders are small. LMK

On Tue, 14 Apr 2020 at 0:01 Bor Kae Hwang ***@***.***> wrote: @andyscott <https://github.com/andyscott> Friendly ping here again (sorry for bothering). We would like to see this moving forward, we are also happy to help if you are busy. Thanks. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#1005 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKQQF6Y65EO476G42NTPF3RMN4TLANCNFSM4KYCEV7A> .

-- *Ittai Zeidman* 40 Hanamal street, Tel Aviv, Israel <http://www.wix.com>

andyscott · 2020-04-29T05:25:44Z

I've merged master and addressed all recent (😆) feedback. Hopefully I didn't miss anything!

ittaiz

👍 merging

* clean up workers * whoops! * git grep --name-only 'String args\[\]' | xargs sed -i 's|String args\[\]|String[] args|' * subclass ByteArrayOutputStream to allow shrinking of internal buffer * gc after each work request * split workerMain implementation into two private methods for readability * add comment about stdin * add a test for the buffer * improve tests * wrap gc and friends in the finally clause * comments * make the worker public * cleanup * comments * add stdin test * clean up test

andyscott requested a review from ittaiz as a code owner February 19, 2020 21:36

googlebot added the cla: yes label Feb 19, 2020

andyscott changed the title ~~clean up worker~~ clean up workers Feb 19, 2020

andyscott commented Feb 19, 2020

View reviewed changes

src/java/io/bazel/rulesscala/scalac/ScalacWorker.java Show resolved Hide resolved

clean up workers

2cc952c

andyscott force-pushed the andyscott/clean-up-worker branch from 99a08a3 to 2cc952c Compare February 19, 2020 23:28

andyscott changed the title ~~clean up workers~~ [wip] clean up workers Feb 19, 2020

whoops!

995975d

andyscott changed the title ~~[wip] clean up workers~~ clean up workers Feb 19, 2020

borkaehw reviewed Feb 20, 2020

View reviewed changes

src/java/io/bazel/rulesscala/coverage/instrumenter/JacocoInstrumenter.java Outdated Show resolved Hide resolved

git grep --name-only 'String args\[\]' | xargs sed -i 's|String args\…

16b4ac3

…[\]|String[] args|'

ittaiz reviewed Feb 21, 2020

View reviewed changes

ittaiz mentioned this pull request Feb 21, 2020

Generic scala_test rule #959

Closed

andyscott added 5 commits February 21, 2020 10:18

subclass ByteArrayOutputStream to allow shrinking of internal buffer

13b2825

gc after each work request

1fe70fc

split workerMain implementation into two private methods for readability

c289a12

add comment about stdin

9e8f673

add a test for the buffer

aba90b5

ittaiz reviewed Feb 22, 2020

View reviewed changes

src/java/io/bazel/rulesscala/worker/Worker.java Outdated Show resolved Hide resolved

src/java/io/bazel/rulesscala/worker/BUILD Outdated Show resolved Hide resolved

src/java/io/bazel/rulesscala/worker/GenericWorker.java Show resolved Hide resolved

andyscott added 6 commits February 22, 2020 16:55

improve tests

f92df24

wrap gc and friends in the finally clause

87104f7

comments

118581f

make the worker public

8ba11b5

cleanup

2bc7a93

comments

ac308e2

add stdin test

2b02012

ittaiz reviewed Mar 7, 2020

View reviewed changes

andyscott added 2 commits April 28, 2020 20:27

Merge branch 'master' into andyscott/clean-up-worker

6f7aed9

clean up test

fe63cf2

ittaiz approved these changes Apr 29, 2020

View reviewed changes

ittaiz merged commit 73c0dbb into bazelbuild:master Apr 29, 2020

andyscott deleted the andyscott/clean-up-worker branch April 29, 2020 16:16

gergelyfabian pushed a commit to gergelyfabian/rules_scala that referenced this pull request May 31, 2022

Add validation test for Kotlin lambda expressions (bazelbuild#1005)

17313eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up workers #1005

clean up workers #1005

andyscott commented Feb 19, 2020 •

edited

Loading

ittaiz commented Feb 20, 2020

andyscott commented Feb 20, 2020

borkaehw left a comment •

edited

Loading

andyscott commented Feb 20, 2020 •

edited

Loading

borkaehw commented Feb 20, 2020

ittaiz commented Feb 21, 2020

ittaiz left a comment

ittaiz Feb 21, 2020

ittaiz Feb 21, 2020

andyscott Feb 21, 2020

andyscott Feb 21, 2020

ittaiz Feb 22, 2020

borkaehw commented Mar 5, 2020

ittaiz commented Mar 5, 2020 via email

borkaehw commented Mar 5, 2020

ittaiz commented Mar 5, 2020

borkaehw commented Mar 6, 2020

ittaiz Feb 22, 2020

ittaiz Mar 7, 2020

andyscott Apr 29, 2020

borkaehw commented Mar 16, 2020

borkaehw commented Apr 13, 2020

ittaiz commented Apr 13, 2020 via email

andyscott commented Apr 29, 2020 •

edited

Loading

ittaiz left a comment

clean up workers #1005

clean up workers #1005

Conversation

andyscott commented Feb 19, 2020 • edited Loading

Description

Motivation

ittaiz commented Feb 20, 2020

andyscott commented Feb 20, 2020

borkaehw left a comment • edited Loading

Choose a reason for hiding this comment

andyscott commented Feb 20, 2020 • edited Loading

borkaehw commented Feb 20, 2020

ittaiz commented Feb 21, 2020

ittaiz left a 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

borkaehw commented Mar 5, 2020

ittaiz commented Mar 5, 2020 via email

borkaehw commented Mar 5, 2020

ittaiz commented Mar 5, 2020

borkaehw commented Mar 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borkaehw commented Mar 16, 2020

borkaehw commented Apr 13, 2020

ittaiz commented Apr 13, 2020 via email

andyscott commented Apr 29, 2020 • edited Loading

ittaiz left a comment

Choose a reason for hiding this comment

andyscott commented Feb 19, 2020 •

edited

Loading

borkaehw left a comment •

edited

Loading

andyscott commented Feb 20, 2020 •

edited

Loading

andyscott commented Apr 29, 2020 •

edited

Loading