Move cli shutdown hook to CliToolLauncher (#86412)

Each Command subclass can implement close() so that resources will be
cleaned up on exceptional exit like SIGINT. This is implemented through
a shutdown hook added in the superclass constructor. However, this hook
makes testing difficult because the hook cannot be added in normal
tests, so a flag must be overriden when testing Command classes.

This commit moves the shutdown hook handling into the CliToolLauncher
that creates the command. It also adds non-evil tests that check how the
hook runs, in place of the old evil tests that actually registered a
real shutdown hook.

relates #85758

distribution/tools/cli-launcher/src/main/java/org/elasticsearch/launcher/CliToolLauncher.java

@@ -17,6 +17,8 @@
 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.core.SuppressForbidden;
 
+import java.io.Closeable;
+import java.io.IOException;
 import java.util.Map;
 
 /**
@@ -53,7 +55,12 @@ public static void main(String[] args) throws Exception {
         String libs = pinfo.sysprops().getOrDefault("cli.libs", "");
 
         Command command = CliToolProvider.load(toolname, libs).create();
-        exit(command.main(args, Terminal.DEFAULT, pinfo));
+        Terminal terminal = Terminal.DEFAULT;
+        Runtime.getRuntime().addShutdownHook(createShutdownHook(terminal, command));
+
+        int exitCode = command.main(args, terminal, pinfo);
+        terminal.flush(); // make sure nothing is left in buffers
+        exit(exitCode);
     }
 
     // package private for tests
@@ -72,6 +79,18 @@ static String getToolName(Map<String, String> sysprops) {
         return toolname;
     }
 
+    static Thread createShutdownHook(Terminal terminal, Closeable closeable) {
+        return new Thread(() -> {
+            try {
+                closeable.close();
+            } catch (final IOException e) {
+                e.printStackTrace(terminal.getErrorWriter());
+            }
+            terminal.flush(); // make sure to flush whatever the close or error might have written
+        });
+
+    }
+
     @SuppressForbidden(reason = "System#exit")
     private static void exit(int status) {
         System.exit(status);

distribution/tools/cli-launcher/src/test/java/org/elasticsearch/launcher/CliToolLauncherTests.java

@@ -8,12 +8,20 @@
 
 package org.elasticsearch.launcher;
 
+import org.elasticsearch.cli.MockTerminal;
 import org.elasticsearch.test.ESTestCase;
 
+import java.io.Closeable;
+import java.io.IOException;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicBoolean;
 
+import static org.elasticsearch.launcher.CliToolLauncher.createShutdownHook;
 import static org.elasticsearch.launcher.CliToolLauncher.getToolName;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.emptyString;
 import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.is;
 
 public class CliToolLauncherTests extends ESTestCase {
 
@@ -30,4 +38,24 @@ public void testScriptNameSyspropWindows() {
         var sysprops = Map.of("cli.name", "", "cli.script", "C:\\foo\\bar\\elasticsearch-mycli.bat", "os.name", "Windows XP");
         assertThat(getToolName(sysprops), equalTo("mycli"));
     }
+
+    public void testShutdownHook() {
+        MockTerminal terminal = MockTerminal.create();
+        AtomicBoolean closeCalled = new AtomicBoolean();
+        Closeable toClose = () -> { closeCalled.set(true); };
+        Thread hook = createShutdownHook(terminal, toClose);
+        hook.run();
+        assertThat(closeCalled.get(), is(true));
+        assertThat(terminal.getErrorOutput(), emptyString());
+    }
+
+    public void testShutdownHookError() {
+        MockTerminal terminal = MockTerminal.create();
+        Closeable toClose = () -> { throw new IOException("something bad happened"); };
+        Thread hook = createShutdownHook(terminal, toClose);
+        hook.run();
+        assertThat(terminal.getErrorOutput(), containsString("something bad happened"));
+        // ensure that we dump the stack trace too
+        assertThat(terminal.getErrorOutput(), containsString("at org.elasticsearch.launcher.CliToolLauncherTests.lambda"));
+    }
 }

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/cli/ListPluginsCommandTests.java

@@ -248,10 +248,6 @@ protected Environment createEnv(OptionSet options, ProcessInfo processInfo) {
                 return env;
             }
 
-            @Override
-            protected boolean addShutdownHook() {
-                return false;
-            }
         };
     }
 }

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/cli/MockInstallPluginCommand.java

@@ -30,8 +30,4 @@ protected Environment createEnv(OptionSet options, ProcessInfo processInfo) thro
         return this.env != null ? this.env : super.createEnv(options, processInfo);
     }
 
-    @Override
-    protected boolean addShutdownHook() {
-        return false;
-    }
 }

distribution/tools/plugin-cli/src/test/java/org/elasticsearch/plugins/cli/RemovePluginActionTests.java

@@ -239,9 +239,6 @@ public void testRemoveUninstalledPluginErrors() throws Exception {
         MockTerminal terminal = MockTerminal.create();
 
         new MockRemovePluginCommand(env) {
-            protected boolean addShutdownHook() {
-                return false;
-            }
         }.main(new String[] { "-Epath.home=" + home, "fake" }, terminal, new ProcessInfo(Map.of(), Map.of(), createTempDir()));
         try (
             BufferedReader reader = new BufferedReader(new StringReader(terminal.getOutput()));

libs/cli/src/main/java/org/elasticsearch/cli/Command.java

@@ -17,8 +17,6 @@
 
 import java.io.Closeable;
 import java.io.IOException;
-import java.io.PrintWriter;
-import java.io.StringWriter;
 import java.util.Arrays;
 
 /**
@@ -46,29 +44,8 @@ public Command(final String description) {
         this.description = description;
     }
 
-    private Thread shutdownHookThread;
-
     /** Parses options for this command from args and executes it. */
     public final int main(String[] args, Terminal terminal, ProcessInfo processInfo) throws Exception {
-        if (addShutdownHook()) {
-
-            shutdownHookThread = new Thread(() -> {
-                try {
-                    this.close();
-                } catch (final IOException e) {
-                    try (StringWriter sw = new StringWriter(); PrintWriter pw = new PrintWriter(sw)) {
-                        e.printStackTrace(pw);
-                        terminal.errorPrintln(sw.toString());
-                    } catch (final IOException impossible) {
-                        // StringWriter#close declares a checked IOException from the Closeable interface but the Javadocs for StringWriter
-                        // say that an exception here is impossible
-                        throw new AssertionError(impossible);
-                    }
-                }
-            });
-            Runtime.getRuntime().addShutdownHook(shutdownHookThread);
-        }
-
         try {
             mainWithoutErrorHandling(args, terminal, processInfo);
         } catch (OptionException e) {
@@ -143,21 +120,6 @@ protected static void exit(int status) {
      * Any runtime user errors (like an input file that does not exist), should throw a {@link UserException}. */
     protected abstract void execute(Terminal terminal, OptionSet options, ProcessInfo processInfo) throws Exception;
 
-    /**
-     * Return whether or not to install the shutdown hook to cleanup resources on exit. This method should only be overridden in test
-     * classes.
-     *
-     * @return whether or not to install the shutdown hook
-     */
-    protected boolean addShutdownHook() {
-        return true;
-    }
-
-    /** Gets the shutdown hook thread if it exists **/
-    Thread getShutdownHookThread() {
-        return shutdownHookThread;
-    }
-
     @Override
     public void close() throws IOException {
 

server/src/test/java/org/elasticsearch/bootstrap/ElasticsearchCliTests.java

@@ -177,10 +177,6 @@ void init(boolean daemonize, Path pidFile, boolean quiet, Environment initialEnv
                 }
             }
 
-            @Override
-            public boolean addShutdownHook() {
-                return false;
-            }
         };
     }
 }

server/src/test/java/org/elasticsearch/cli/CommandTests.java

@@ -53,11 +53,6 @@ protected void printAdditionalHelp(Terminal terminal) {
             terminal.println("Some extra help");
         }
 
-        @Override
-        protected boolean addShutdownHook() {
-            return false;
-        }
-
     }
 
     public void testHelp() throws Exception {

server/src/test/java/org/elasticsearch/cli/MultiCommandTests.java

@@ -41,10 +41,6 @@ public void close() throws IOException {
             }
         }
 
-        @Override
-        protected boolean addShutdownHook() {
-            return false;
-        }
     }
 
     static class DummySubCommand extends Command {
@@ -211,10 +207,6 @@ protected void execute(Terminal terminal, OptionSet options, ProcessInfo process
             throw new UserException(1, "Dummy error");
         }
 
-        @Override
-        protected boolean addShutdownHook() {
-            return false;
-        }
     }
 
     public void testErrorDisplayedWithDefault() throws Exception {

x-pack/plugin/sql/qa/server/src/main/java/org/elasticsearch/xpack/sql/qa/cli/EmbeddedCli.java

@@ -77,10 +77,6 @@ public EmbeddedCli(String elasticsearchAddress, boolean checkConnectionOnStartup
             false
         );
         cli = new Cli(cliTerminal) {
-            @Override
-            protected boolean addShutdownHook() {
-                return false;
-            }
         };
         out = new BufferedWriter(new OutputStreamWriter(outgoing, StandardCharsets.UTF_8));
         in = new BufferedReader(new InputStreamReader(incoming, StandardCharsets.UTF_8));