Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove user exception marker from server cli #93206

Merged
merged 2 commits into from
Jan 24, 2023
Merged

Remove user exception marker from server cli #93206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2534671
Remove user exception marker from server cli
rjernst Jan 24, 2023
3bfaed0
actually remove the marker
rjernst Jan 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 4 additions & 19 deletions ...ribution/tools/server-cli/src/main/java/org/elasticsearch/server/cli/ErrorPumpThread.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,14 @@
import java.util.concurrent.CountDownLatch;

import static org.elasticsearch.bootstrap.BootstrapInfo.SERVER_READY_MARKER;
import static org.elasticsearch.bootstrap.BootstrapInfo.USER_EXCEPTION_MARKER;
thecoop marked this conversation as resolved.
Show resolved Hide resolved
import static org.elasticsearch.server.cli.ProcessUtil.nonInterruptibleVoid;

/**
* A thread which reads stderr of the jvm process and writes it to this process' stderr.
*
* <p> Two special state markers are watched for. These are ascii control characters which signal
* to the cli process something has changed in the server process. The two possible special messages are:
* <ul>
* <li>{@link BootstrapInfo#USER_EXCEPTION_MARKER} - signals a bootstrap error has occurred, and is followed
* by the error message</li>
* <li>{@link BootstrapInfo#SERVER_READY_MARKER} - signals the server is ready so the cli may detach if daemonizing</li>
* </ul>
* <p> The thread watches for a special state marker from the process. The ascii character
* {@link BootstrapInfo#SERVER_READY_MARKER} signals the server is ready and the cli may
* detach if daemonizing. All other messages are passed through to stderr.
*/
class ErrorPumpThread extends Thread {
private final BufferedReader reader;
Expand All @@ -43,9 +38,6 @@ class ErrorPumpThread extends Thread {
// a flag denoting whether the ready marker has been received by the server process
private volatile boolean ready;

// an exception message received alongside the user exception marker, if a bootstrap error has occurred
private volatile String userExceptionMsg;

// an unexpected io failure that occurred while pumping stderr
private volatile IOException ioFailure;

Expand All @@ -66,10 +58,6 @@ String waitUntilReady() throws IOException {
if (ioFailure != null) {
throw ioFailure;
}
if (ready == false) {
return userExceptionMsg;
}
assert userExceptionMsg == null;
return null;
}

Expand All @@ -85,10 +73,7 @@ public void run() {
try {
String line;
while ((line = reader.readLine()) != null) {
if (line.isEmpty() == false && line.charAt(0) == USER_EXCEPTION_MARKER) {
userExceptionMsg = line.substring(1);
readyOrDead.countDown();
} else if (line.isEmpty() == false && line.charAt(0) == SERVER_READY_MARKER) {
if (line.isEmpty() == false && line.charAt(0) == SERVER_READY_MARKER) {
ready = true;
readyOrDead.countDown();
} else {
Expand Down
22 changes: 6 additions & 16 deletions ...ution/tools/server-cli/src/test/java/org/elasticsearch/server/cli/ServerProcessTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,9 +78,9 @@ interface ProcessValidator {
void validate(ProcessBuilder processBuilder) throws IOException;
}

void runForeground() throws Exception {
int runForeground() throws Exception {
var server = startProcess(false, false, "");
server.waitFor();
return server.waitFor();
}

@Before
Expand Down Expand Up @@ -228,22 +228,12 @@ public void testPid() throws Exception {

public void testBootstrapError() throws Exception {
mainCallback = (args, stdin, stderr, exitCode) -> {
stderr.println(BootstrapInfo.USER_EXCEPTION_MARKER + "a bootstrap exception");
stderr.println("a bootstrap exception");
exitCode.set(ExitCodes.CONFIG);
};
var e = expectThrows(UserException.class, () -> runForeground());
assertThat(e.exitCode, equalTo(ExitCodes.CONFIG));
assertThat(e.getMessage(), equalTo("a bootstrap exception"));
}

public void testUserError() throws Exception {
mainCallback = (args, stdin, stderr, exitCode) -> {
stderr.println(BootstrapInfo.USER_EXCEPTION_MARKER + "a user exception");
exitCode.set(ExitCodes.USAGE);
};
var e = expectThrows(UserException.class, () -> runForeground());
assertThat(e.exitCode, equalTo(ExitCodes.USAGE));
assertThat(e.getMessage(), equalTo("a user exception"));
int exitCode = runForeground();
assertThat(exitCode, equalTo(ExitCodes.CONFIG));
assertThat(terminal.getErrorOutput(), containsString("a bootstrap exception"));
}

public void testStartError() throws Exception {
Expand Down
7 changes: 0 additions & 7 deletions server/src/main/java/org/elasticsearch/bootstrap/BootstrapInfo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,6 @@ public static ConsoleLoader.Console getConsole() {
*/
public static final String UNTRUSTED_CODEBASE = "/untrusted";

/**
* A non-printable character denoting a UserException has occurred.
*
* This is sent over stderr to the controlling CLI process.
*/
public static final char USER_EXCEPTION_MARKER = '\u0015';

/**
* A non-printable character denoting the server is ready to process requests.
*
Expand Down