8338281: jshell does not run shutdown hooks

lahodaj · Aug 14, 2024 · d5f6eba · d5f6eba
1 parent 38bd8a3
commit d5f6eba
Show file tree

Hide file tree

Showing 4 changed files with 55 additions and 4 deletions.
diff --git a/src/jdk.jshell/share/classes/jdk/jshell/execution/ExecutionControlForwarder.java b/src/jdk.jshell/share/classes/jdk/jshell/execution/ExecutionControlForwarder.java
@@ -177,7 +177,7 @@ private boolean processCommand() throws IOException {
                     } catch (Throwable ex) {
                         // JShell-core not waiting for a result, ignore
                     }
-                    return true;
+                    return false;
                 }
                 default: {
                     Object arg = in.readObject();

diff --git a/src/jdk.jshell/share/classes/jdk/jshell/execution/JdiDefaultExecutionControl.java b/src/jdk.jshell/share/classes/jdk/jshell/execution/JdiDefaultExecutionControl.java
@@ -49,6 +49,7 @@
 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;
@@ -267,6 +268,13 @@ public void stop() throws EngineTerminationException, InternalException {
     @Override
     public void close() {
         super.close();
+        if (process != null) {
+            try {
+                process.waitFor(1, TimeUnit.SECONDS);
+            } catch (InterruptedException ex) {
+                debug(ex, "waitFor remote");
+            }
+        }
         disposeVM();
     }
 

diff --git a/test/langtools/jdk/jshell/KullaTesting.java b/test/langtools/jdk/jshell/KullaTesting.java
@@ -167,8 +167,12 @@ public void addToClasspath(Path path) {
     }
 
     @BeforeMethod
-    public void setUp() {
-        setUp(b -> {});
+    public void setUp(Method testMethod) {
+        setUp(testMethod, b -> {});
+    }
+
+    public void setUp(Method testMethod, Consumer<JShell.Builder> bc) {
+        setUp(bc);
     }
 
     public void setUp(Consumer<JShell.Builder> bc) {

diff --git a/test/langtools/jdk/jshell/ShutdownTest.java b/test/langtools/jdk/jshell/ShutdownTest.java
@@ -28,15 +28,20 @@
  * @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;
 import jdk.jshell.JShell.Subscription;
 import org.testng.annotations.Test;
 
 import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
 
-@Test
 public class ShutdownTest extends KullaTesting {
 
     int shutdownCount;
@@ -53,13 +58,15 @@ public void testExit() {
         assertEquals(shutdownCount, 1);
     }
 
+    @Test
     public void testCloseCallback() {
         shutdownCount = 0;
         getState().onShutdown(this::shutdownCounter);
         getState().close();
         assertEquals(shutdownCount, 1);
     }
 
+    @Test
     public void testCloseUnsubscribe() {
         shutdownCount = 0;
         Subscription token = getState().onShutdown(this::shutdownCounter);
@@ -68,6 +75,7 @@ public void testCloseUnsubscribe() {
         assertEquals(shutdownCount, 0);
     }
 
+    @Test
     public void testTwoShutdownListeners() {
         ShutdownListener listener1 = new ShutdownListener();
         ShutdownListener listener2 = new ShutdownListener();
@@ -118,6 +126,37 @@ public void testSubscriptionAfterShutdown() {
         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()));
+        getState().close();
+        assertFalse(Files.exists(temporary));
+    }
+
+    @Override
+    public void setUp(Method testMethod, Consumer<JShell.Builder> bc) {
+        Consumer<JShell.Builder> augmentedBuilder = switch (testMethod.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;