8157917: JShell: shutdown could cause remote JDWP errors to be visible to users

8157918: JShell tests: StartOptionTest displays insufficient information to diagnose failures

Reviewed-by: vromero

langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/jdi/JDIConnection.java

@@ -50,6 +50,7 @@ import static jdk.internal.jshell.debug.InternalDebugControl.DBG_GEN;
 class JDIConnection {
 
     private VirtualMachine vm;
+    private boolean active = true;
     private Process process = null;
     private int outputCompleteCount = 0;
 
@@ -175,6 +176,11 @@ class JDIConnection {
         return process != null && process.isAlive();
     }
 
+    // Beginning shutdown, ignore any random dying squeals
+    void beginShutdown() {
+        active = false;
+    }
+
     public synchronized void disposeVM() {
         try {
             if (vm != null) {
@@ -233,14 +239,19 @@ class JDIConnection {
         int i;
         try {
             while ((i = in.read()) != -1) {
-                pStream.print((char) i);
+                // directly copy input to output, but skip if asked to close
+                if (active) {
+                    pStream.print((char) i);
+                }
             }
         } catch (IOException ex) {
             String s = ex.getMessage();
-            if (!s.startsWith("Bad file number")) {
+            if (active && !s.startsWith("Bad file number")) {
                 throw ex;
             }
-            // else we got a Bad file number IOException which just means
+            // else we are being shutdown (and don't want any spurious death
+            // throws to ripple) or
+            // we got a Bad file number IOException which just means
             // that the debuggee has gone away.  We'll just treat it the
             // same as if we got an EOF.
         }

langtools/src/jdk.jshell/share/classes/jdk/internal/jshell/jdi/JDIExecutionControl.java

@@ -109,11 +109,14 @@ public class JDIExecutionControl implements ExecutionControl {
     @Override
     public void close() {
         try {
+            JDIConnection c = jdiEnv.connection();
+            if (c != null) {
+                c.beginShutdown();
+            }
             if (remoteOut != null) {
                 remoteOut.writeInt(CMD_EXIT);
                 remoteOut.flush();
             }
-            JDIConnection c = jdiEnv.connection();
             if (c != null) {
                 c.disposeVM();
             }

langtools/test/jdk/jshell/StartOptionTest.java

@@ -34,7 +34,6 @@
  */
 
 import java.io.ByteArrayOutputStream;
-import java.io.OutputStream;
 import java.io.PrintStream;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Path;
@@ -48,85 +47,76 @@ import org.testng.annotations.Test;
 
 import static org.testng.Assert.assertEquals;
 import static org.testng.Assert.assertTrue;
-import static org.testng.Assert.fail;
 
 @Test
 public class StartOptionTest {
 
-    private ByteArrayOutputStream out;
+    private ByteArrayOutputStream cmdout;
-    private ByteArrayOutputStream err;
+    private ByteArrayOutputStream cmderr;
+    private ByteArrayOutputStream console;
+    private ByteArrayOutputStream userout;
+    private ByteArrayOutputStream usererr;
 
     private JShellTool getShellTool() {
-        class NoOutputAllowedStream extends OutputStream {
-            private final String label;
-            NoOutputAllowedStream(String label) {
-               this.label = label;
-            }
-            @Override
-            public void write(int b) { fail("Unexpected output to: " + label); }
-        }
         return new JShellTool(
                 new TestingInputStream(),
-                new PrintStream(out),
+                new PrintStream(cmdout),
-                new PrintStream(err),
+                new PrintStream(cmderr),
-                new PrintStream(new NoOutputAllowedStream("console")),
+                new PrintStream(console),
                 new TestingInputStream(),
-                new PrintStream(new NoOutputAllowedStream("userout")),
+                new PrintStream(userout),
-                new PrintStream(new NoOutputAllowedStream("usererr")),
+                new PrintStream(usererr),
                 new ReplToolTesting.MemoryPreferences(),
                 Locale.ROOT);
     }
 
-    private String getOutput() {
+    private void check(ByteArrayOutputStream str, Consumer<String> checkOut, String label) {
-        byte[] bytes = out.toByteArray();
+        byte[] bytes = str.toByteArray();
-        out.reset();
+        str.reset();
-        return new String(bytes, StandardCharsets.UTF_8);
+        String out =  new String(bytes, StandardCharsets.UTF_8);
-    }
+        if (checkOut != null) {
-
+            checkOut.accept(out);
-    private String getError() {
+        } else {
-        byte[] bytes = err.toByteArray();
+            assertEquals("", out, label + ": Expected empty -- ");
-        err.reset();
+        }
-        return new String(bytes, StandardCharsets.UTF_8);
     }
 
     private void start(Consumer<String> checkOutput, Consumer<String> checkError, String... args) throws Exception {
         JShellTool tool = getShellTool();
         tool.start(args);
-        if (checkOutput != null) {
+        check(cmdout, checkOutput, "cmdout");
-            checkOutput.accept(getOutput());
+        check(cmderr, checkError, "cmderr");
-        } else {
+        check(console, null, "console");
-            assertEquals("", getOutput(), "Output: ");
+        check(userout, null, "userout");
-        }
+        check(usererr, null, "usererr");
-        if (checkError != null) {
-            checkError.accept(getError());
-        } else {
-            assertEquals("", getError(), "Error: ");
-        }
     }
 
     private void start(String expectedOutput, String expectedError, String... args) throws Exception {
-        start(s -> assertEquals(s.trim(), expectedOutput, "Output: "), s -> assertEquals(s.trim(), expectedError, "Error: "), args);
+        start(s -> assertEquals(s.trim(), expectedOutput, "cmdout: "), s -> assertEquals(s.trim(), expectedError, "cmderr: "), args);
     }
 
     @BeforeMethod
     public void setUp() {
-        out = new ByteArrayOutputStream();
+        cmdout  = new ByteArrayOutputStream();
-        err = new ByteArrayOutputStream();
+        cmderr  = new ByteArrayOutputStream();
+        console = new ByteArrayOutputStream();
+        userout = new ByteArrayOutputStream();
+        usererr = new ByteArrayOutputStream();
     }
 
     @Test
     public void testUsage() throws Exception {
         start(s -> {
-            assertTrue(s.split("\n").length >= 7, s);
+            assertTrue(s.split("\n").length >= 7, "Not enough usage lines: " + s);
-            assertTrue(s.startsWith("Usage:   jshell <options>"), s);
+            assertTrue(s.startsWith("Usage:   jshell <options>"), "Unexpect usage start: " + s);
         },  null, "-help");
     }
 
     @Test
     public void testUnknown() throws Exception {
         start(s -> {
-            assertTrue(s.split("\n").length >= 7, s);
+            assertTrue(s.split("\n").length >= 7, "Not enough usage lines (unknown): " + s);
-            assertTrue(s.startsWith("Usage:   jshell <options>"), s);
+            assertTrue(s.startsWith("Usage:   jshell <options>"), "Unexpect usage start (unknown): " + s);
         }, s -> assertEquals(s.trim(), "Unknown option: -unknown"), "-unknown");
     }
 
@@ -157,12 +147,15 @@ public class StartOptionTest {
 
     @Test
     public void testVersion() throws Exception {
-        start(s -> assertTrue(s.startsWith("jshell")), null, "-version");
+        start(s -> assertTrue(s.startsWith("jshell"), "unexpected version: " + s), null, "-version");
     }
 
     @AfterMethod
     public void tearDown() {
-        out = null;
+        cmdout  = null;
-        err = null;
+        cmderr  = null;
+        console = null;
+        userout = null;
+        usererr = null;
     }
 }