8338281: jshell does not run shutdown hooks

Reviewed-by: asotona

src/jdk.jshell/share/classes/jdk/jshell/execution/ExecutionControlForwarder.java

@@ -177,7 +177,7 @@ class ExecutionControlForwarder {
                     } catch (Throwable ex) {
                         // JShell-core not waiting for a result, ignore
                     }
-                    return true;
+                    return false;
                 }
                 default: {
                     Object arg = in.readObject();

src/jdk.jshell/share/classes/jdk/jshell/execution/JdiDefaultExecutionControl.java

@@ -49,6 +49,7 @@ import com.sun.jdi.VMDisconnectedException;
 import com.sun.jdi.VirtualMachine;
 import java.io.PrintStream;
 import java.util.Optional;
+import java.util.concurrent.TimeUnit;
 import java.util.stream.Stream;
 import jdk.jshell.JShellConsole;
 import jdk.jshell.execution.JdiDefaultExecutionControl.JdiStarter.TargetDescription;
@@ -70,6 +71,8 @@ import jdk.jshell.execution.impl.ConsoleImpl.ConsoleOutputStream;
  */
 public class JdiDefaultExecutionControl extends JdiExecutionControl {
 
+    private static final int SHUTDOWN_TIMEOUT = 1; //1 second
+
     private VirtualMachine vm;
     private Process process;
     private final String remoteAgent;
@@ -267,6 +270,20 @@ public class JdiDefaultExecutionControl extends JdiExecutionControl {
     @Override
     public void close() {
         super.close();
+
+        Process remoteProcess;
+
+        synchronized (this) {
+            remoteProcess = this.process;
+        }
+
+        if (remoteProcess != null) {
+            try {
+                remoteProcess.waitFor(SHUTDOWN_TIMEOUT, TimeUnit.SECONDS);
+            } catch (InterruptedException ex) {
+                debug(ex, "waitFor remote");
+            }
+        }
         disposeVM();
     }
 

test/langtools/jdk/jshell/ShutdownTest.java

@@ -28,6 +28,11 @@
  * @run testng ShutdownTest
  */
 
+import java.io.IOException;
+import java.lang.reflect.Method;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.function.Consumer;
 
 import jdk.jshell.JShell;
@@ -35,8 +40,9 @@ import jdk.jshell.JShell.Subscription;
 import org.testng.annotations.Test;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import org.testng.annotations.BeforeMethod;
 
-@Test
 public class ShutdownTest extends KullaTesting {
 
     int shutdownCount;
@@ -53,6 +59,7 @@ public class ShutdownTest extends KullaTesting {
         assertEquals(shutdownCount, 1);
     }
 
+    @Test
     public void testCloseCallback() {
         shutdownCount = 0;
         getState().onShutdown(this::shutdownCounter);
@@ -60,6 +67,7 @@ public class ShutdownTest extends KullaTesting {
         assertEquals(shutdownCount, 1);
     }
 
+    @Test
     public void testCloseUnsubscribe() {
         shutdownCount = 0;
         Subscription token = getState().onShutdown(this::shutdownCounter);
@@ -68,6 +76,7 @@ public class ShutdownTest extends KullaTesting {
         assertEquals(shutdownCount, 0);
     }
 
+    @Test
     public void testTwoShutdownListeners() {
         ShutdownListener listener1 = new ShutdownListener();
         ShutdownListener listener2 = new ShutdownListener();
@@ -118,6 +127,51 @@ public class ShutdownTest extends KullaTesting {
         getState().onShutdown(e -> {});
     }
 
+    @Test
+    public void testRunShutdownHooks() throws IOException {
+        Path temporary = Paths.get("temp");
+        Files.newOutputStream(temporary).close();
+        assertEval("import java.io.*;");
+        assertEval("import java.nio.file.*;");
+        assertEval("""
+                        Runtime.getRuntime().addShutdownHook(new Thread(() -> {
+                            try {
+                                Files.delete(Paths.get("$TEMPORARY"));
+                            } catch (IOException ex) {
+                                //ignored
+                            }
+                        }))
+                        """.replace("$TEMPORARY", temporary.toAbsolutePath()
+                                                           .toString()
+                                                           .replace("\\", "\\\\")));
+        getState().close();
+        assertFalse(Files.exists(temporary));
+    }
+
+    private Method currentTestMethod;
+
+    @BeforeMethod
+    public void setUp(Method testMethod) {
+        currentTestMethod = testMethod;
+        super.setUp();
+    }
+
+    @BeforeMethod
+    public void setUp() {
+    }
+
+    @Override
+    public void setUp(Consumer<JShell.Builder> bc) {
+        Consumer<JShell.Builder> augmentedBuilder = switch (currentTestMethod.getName()) {
+            case "testRunShutdownHooks" -> builder -> {
+                builder.executionEngine(Presets.TEST_STANDARD_EXECUTION);
+                bc.accept(builder);
+            };
+            default -> bc;
+        };
+        super.setUp(augmentedBuilder);
+    }
+
     private static class ShutdownListener implements Consumer<JShell> {
         private int count;