Skip to content

Commit

Permalink
Prevent unexpected native controller output hanging the process (elas…
Browse files Browse the repository at this point in the history
…tic#56688)

In normal operation native controllers are not expected to write
anything to stdout or stderr.  However, if due to an error or
something unexpected with the environment a native controller
does write something to stdout or stderr then it will block if
nothing is reading that output.

This change makes the stdout and stderr of native controllers
reuse the same stdout and stderr as the Elasticsearch JVM (which
are by default redirected to es.stdout.log and es.stderr.log) so
that if something unexpected is written to native controller
output then:

1. The native controller process does not block, waiting for
   something to read the output
2. We can see what the output was, making it easier to debug
   obscure environmental problems

Backport of elastic#56491
  • Loading branch information
droberts195 authored May 14, 2020
1 parent b237f57 commit dec7c21
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ public void testNoControllerSpawn() throws IOException {
"has.native.controller", "false");

try (Spawner spawner = new Spawner()) {
spawner.spawnNativeControllers(environment);
spawner.spawnNativeControllers(environment, false);
assertThat(spawner.getProcesses(), hasSize(0));
}
}
Expand Down Expand Up @@ -149,7 +149,7 @@ private void assertControllerSpawns(final Function<Environment, Path> pluginsDir
"has.native.controller", "false");

Spawner spawner = new Spawner();
spawner.spawnNativeControllers(environment);
spawner.spawnNativeControllers(environment, false);

List<Process> processes = spawner.getProcesses();

Expand Down Expand Up @@ -196,7 +196,7 @@ public void testControllerSpawnWithIncorrectDescriptor() throws IOException {
Spawner spawner = new Spawner();
IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> spawner.spawnNativeControllers(environment));
() -> spawner.spawnNativeControllers(environment, false));
assertThat(
e.getMessage(),
equalTo("module [test_plugin] does not have permission to fork native controller"));
Expand All @@ -217,10 +217,10 @@ public void testSpawnerHandlingOfDesktopServicesStoreFiles() throws IOException
final Spawner spawner = new Spawner();
if (Constants.MAC_OS_X) {
// if the spawner were not skipping the Desktop Services Store files on macOS this would explode
spawner.spawnNativeControllers(environment);
spawner.spawnNativeControllers(environment, false);
} else {
// we do not ignore these files on non-macOS systems
final FileSystemException e = expectThrows(FileSystemException.class, () -> spawner.spawnNativeControllers(environment));
final FileSystemException e = expectThrows(FileSystemException.class, () -> spawner.spawnNativeControllers(environment, false));
if (Constants.WINDOWS) {
assertThat(e, instanceOf(NoSuchFileException.class));
} else {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ private void setup(boolean addShutdownHook, Environment environment) throws Boot
Settings settings = environment.settings();

try {
spawner.spawnNativeControllers(environment);
spawner.spawnNativeControllers(environment, true);
} catch (IOException e) {
throw new BootstrapException(e);
}
Expand Down
18 changes: 14 additions & 4 deletions server/src/main/java/org/elasticsearch/bootstrap/Spawner.java
Original file line number Diff line number Diff line change
Expand Up @@ -55,10 +55,12 @@ public void close() throws IOException {
/**
* Spawns the native controllers for each module.
*
* @param environment the node environment
* @param environment The node environment
* @param inheritIo Should the stdout and stderr of the spawned process inherit the
* stdout and stderr of the JVM spawning it?
* @throws IOException if an I/O error occurs reading the module or spawning a native process
*/
void spawnNativeControllers(final Environment environment) throws IOException {
void spawnNativeControllers(final Environment environment, final boolean inheritIo) throws IOException {
if (!spawned.compareAndSet(false, true)) {
throw new IllegalStateException("native controllers already spawned");
}
Expand All @@ -83,7 +85,7 @@ void spawnNativeControllers(final Environment environment) throws IOException {
modules.getFileName());
throw new IllegalArgumentException(message);
}
final Process process = spawnNativeController(spawnPath, environment.tmpFile());
final Process process = spawnNativeController(spawnPath, environment.tmpFile(), inheritIo);
processes.add(process);
}
}
Expand All @@ -92,7 +94,7 @@ void spawnNativeControllers(final Environment environment) throws IOException {
* Attempt to spawn the controller daemon for a given module. The spawned process will remain connected to this JVM via its stdin,
* stdout, and stderr streams, but the references to these streams are not available to code outside this package.
*/
private Process spawnNativeController(final Path spawnPath, final Path tmpPath) throws IOException {
private Process spawnNativeController(final Path spawnPath, final Path tmpPath, final boolean inheritIo) throws IOException {
final String command;
if (Constants.WINDOWS) {
/*
Expand All @@ -114,6 +116,14 @@ private Process spawnNativeController(final Path spawnPath, final Path tmpPath)
pb.environment().clear();
pb.environment().put("TMPDIR", tmpPath.toString());

// The process _shouldn't_ write any output via its stdout or stderr, but if it does then
// it will block if nothing is reading that output. To avoid this we can inherit the
// JVM's stdout and stderr (which are redirected to files in standard installations).
if (inheritIo) {
pb.redirectOutput(ProcessBuilder.Redirect.INHERIT);
pb.redirectError(ProcessBuilder.Redirect.INHERIT);
}

// the output stream of the process object corresponds to the daemon's stdin
return pb.start();
}
Expand Down

0 comments on commit dec7c21

Please sign in to comment.