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.

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

Don't close stderr under --quiet #47208

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
4 changes: 4 additions & 0 deletions libs/cli/src/main/java/org/elasticsearch/cli/Terminal.java
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,10 @@ public final boolean promptYesNo(String prompt, boolean defaultYes) {
}
}

public void flush() {
this.getErrorWriter().flush();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's puzzling to me that flush only flushes the error stream but not the output stream?

}

private static class ConsoleTerminal extends Terminal {

private static final Console CONSOLE = System.console();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,10 +346,6 @@ static void init(
}

INSTANCE.start();

if (closeStandardStreams) {
closeSysError();
}
} catch (NodeValidationException | RuntimeException e) {
// disable console logging, so user does not see the exception twice (jvm will show it already)
final Logger rootLogger = LogManager.getRootLogger();
Expand Down Expand Up @@ -397,11 +393,6 @@ private static void closeSystOut() {
System.out.close();
}

@SuppressForbidden(reason = "System#err")
private static void closeSysError() {
System.err.close();
}

private static void checkLucene() {
if (Version.CURRENT.luceneVersion.equals(org.apache.lucene.util.Version.LATEST) == false) {
throw new AssertionError("Lucene version mismatch this version of Elasticsearch requires lucene version ["
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,12 @@ public void checkPermission(Permission perm) {
final Elasticsearch elasticsearch = new Elasticsearch();
int status = main(args, elasticsearch, Terminal.DEFAULT);
if (status != ExitCodes.OK) {
Terminal.DEFAULT.errorPrintln("ERROR: Elasticsearch did not exit normally - check the logs at " +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's possible that we can fail before these system properties have been set.

Copy link
Member

@jasontedor jasontedor Oct 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem here is that we can fail before we’ve configured logging, and so those system properties would not be set giving an empty error message to the user. The guarantee that we have here though is that if logging is not yet been configured, then no log messages will have been written (we have a check that ensures we don't touch logging before it's configured, and we fail startup if it is). That means that we should skip these message if the logging properties are not set, because we know from our guarantees that there won't be anything in the logs to look at.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jasontedor I've added guards around both places where I'm printing the log file location.

System.getProperty("es.logs.base_path") +
System.getProperty("file.separator") +
System.getProperty("es.logs.cluster_name") +
".log"
);
exit(status);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.cli.Terminal;
import org.elasticsearch.common.SuppressForbidden;

import java.io.IOError;
Expand All @@ -32,29 +32,29 @@ class ElasticsearchUncaughtExceptionHandler implements Thread.UncaughtExceptionH
private static final Logger logger = LogManager.getLogger(ElasticsearchUncaughtExceptionHandler.class);

@Override
public void uncaughtException(Thread t, Throwable e) {
if (isFatalUncaught(e)) {
public void uncaughtException(Thread thread, Throwable t) {
if (isFatalUncaught(t)) {
try {
onFatalUncaught(t.getName(), e);
onFatalUncaught(thread.getName(), t);
} finally {
// we use specific error codes in case the above notification failed, at least we
// will have some indication of the error bringing us down
if (e instanceof InternalError) {
if (t instanceof InternalError) {
halt(128);
} else if (e instanceof OutOfMemoryError) {
} else if (t instanceof OutOfMemoryError) {
halt(127);
} else if (e instanceof StackOverflowError) {
} else if (t instanceof StackOverflowError) {
halt(126);
} else if (e instanceof UnknownError) {
} else if (t instanceof UnknownError) {
halt(125);
} else if (e instanceof IOError) {
} else if (t instanceof IOError) {
halt(124);
} else {
halt(1);
}
}
} else {
onNonFatalUncaught(t.getName(), e);
onNonFatalUncaught(thread.getName(), t);
}
}

Expand All @@ -63,11 +63,21 @@ static boolean isFatalUncaught(Throwable e) {
}

void onFatalUncaught(final String threadName, final Throwable t) {
logger.error(() -> new ParameterizedMessage("fatal error in thread [{}], exiting", threadName), t);
final String message = "fatal error in thread [" + threadName + "], exiting";
logger.error(message, t);
Terminal.DEFAULT.errorPrintln(message);
t.printStackTrace(Terminal.DEFAULT.getErrorWriter());
// Without a final flush, the stacktrace may not be shown before ES exits
Terminal.DEFAULT.flush();
}

void onNonFatalUncaught(final String threadName, final Throwable t) {
logger.warn(() -> new ParameterizedMessage("uncaught exception in thread [{}]", threadName), t);
final String message = "uncaught exception in thread [" + threadName + "]";
logger.error(message, t);
Terminal.DEFAULT.errorPrintln(message);
t.printStackTrace(Terminal.DEFAULT.getErrorWriter());
// Without a final flush, the stacktrace may not be shown if ES goes on to exit
Terminal.DEFAULT.flush();
}

void halt(int status) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,11 @@ private void printStackTrace(Consumer<String> consumer) {
// if its a guice exception, the whole thing really will not be in the log, its megabytes.
// refer to the hack in bootstrap, where we don't log it
if (originalCause instanceof CreationException == false) {
consumer.accept("Refer to the log for complete error details.");
final String logPath = System.getProperty("es.logs.base_path") +
System.getProperty("file.separator") +
System.getProperty("es.logs.cluster_name") +
".log";
consumer.accept("For complete error details, refer to the log at " + logPath);
}
}

Expand Down