Revamp the test and run fork process infrastructure #449
Conversation
jvican
force-pushed
the
topic/fix-input-process
branch
from
93d8afc
to
0a306ab
Compare
|
The logic in this pull request also makes Bloop able to cancel tests and applications run reliably. |
|
Looks like CI fails because of an annoying bug in Docker. |
|
Modifying the drone test agents to allow SYS_ADMIN seccomp capabilities fixed this issue. CI is now passing. |
jvican
force-pushed
the
topic/fix-input-process
branch
2 times, most recently
from
f470a17
to
bf3fbd1
Compare
jvican
added
the
blocked
Otherwise, `System.console() == null`.
1. Migrate to `NuProcess` and avoid the annoying Java and Scala process APIs. Nuprocess gives us a better foundation to deal with stdout and stderr, as well as processing standard input from the host process (Bloop). `NuProcess` will also be more efficient than our previous approach. 2. Migrate test server and the whole `runMain` machinery to `Task` so that we can have a finer control over it. Note that now these actions can be cancelled!
It fails spuriously with good reason: Bloop doesn't yet support concurrent clients. When we do, we'll re-enable this test suite.
jvican
force-pushed
the
topic/fix-input-process
branch
7 times, most recently
from
ddcf694
to
b6c8473
Compare
The modernization makes sure that: 1. All processes that call bloop do provide an empty or inherited input stream in both the server part and the client part. 2. The implementation is task based so that we can remove the annoying thread sleep + have automatic shutdown of the processes if the timeout is not met.
jvican
force-pushed
the
topic/fix-input-process
branch
from
b6c8473
to
51455d5
Compare
There was a problem hiding this comment.
After an arduous journey to make nuprocess work (and thanks to the great job of the maintainer who cut a new version of nuprocess fixing a fundamental issue so that we can run it in our infrastructure), we're done here.
The PR as is does the following things:
- Implement a task-based
NailgunTestthat:
- Removes an unnecessary big sleep of 500ms for every server init.
- Allows for a timeout (good to identify issues with tests without getting hanging processes).
- Implement a new forker implementation that is task-based and:
- Allows cancelling run and test execution (as proved by the new tests).
- Allows a more efficient handling of process streams.
This PR also disables a test case that doesn't make sense until we have safe concurrent client processing implemented in the core. Then, we'll be able to re-enable it and experiment with parallel nailgun testing.
| @@ -257,7 +257,6 @@ object Cli { | |||
| .flatMap(start => taskState.materialize.map(s => (s, start))) | |||
| .map { case (state, start) => logElapsed(start); state } | |||
| .dematerialize | |||
| .executeWithOptions(_.enableAutoCancelableRunLoops) | |||
There was a problem hiding this comment.
We don't need cancel to propagate through the current flatMap stack, so we disable this. I've also experienced some weird Monix behaviour when this is enabled. Since this is not a well-tested feature and it doesn't make much sense to have it enabled by default, I'm more comfortable with using the stock task options.
|
Oh, and I forgot to say that now we're handling input streams correctly (which means that run applications that require stdin will work). |
I think this is one of the reasons why our nailgun-based test infrastructure has been traditionally not very stable. Let's see if this fixes it.
jvican
force-pushed
the
topic/fix-input-process
branch
2 times, most recently
from
b88b0a7
to
f7cf859
Compare
jvican
force-pushed
the
topic/fix-input-process
branch
from
f7cf859
to
3175510
Compare
Migrate to
NuProcessand avoid the annoying Java and Scala process APIs.Nuprocess gives us a better foundation to deal with stdout and stderr, as well
as processing standard input from the host process (Bloop).
NuProcesswillalso be more efficient than our previous approach.
Migrate test server and the whole
runMainmachinery toTaskso that wecan have a finer control over it. Note that now these actions can be cancelled!