refactor and simplify disposing debugger connection in a thread safe manner

Author: javeleon

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/api/VMEventListenerImpl.java

@@ -904,8 +904,7 @@ public boolean vmDied() {
         }
         stream.writeByte(RequestedJDWPEvents.VM_DEATH);
         stream.writeInt(0);
-        // don't queue this packet, send immediately
-        connection.sendVMDied(stream, debuggerController);
+        connection.queuePacket(stream);
         return vmDeathSuspendPolicy != SuspendStrategy.NONE;
     }
 

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/DebuggerConnection.java

@@ -42,25 +42,19 @@ private DebuggerConnection(SocketConnection connection, DebuggerController contr
         this.context = controller.getContext();
     }
 
-    static void establishDebuggerConnection(DebuggerController controller, DebuggerController.SetupState setupState, boolean isReconnect, CountDownLatch latch) {
-        Thread jdwpReceiver = new Thread(new JDWPReceiver(controller, setupState, isReconnect, latch), "jdwp-receiver");
-        controller.addDebuggerThread(jdwpReceiver);
+    static void establishDebuggerConnection(DebuggerController controller, DebuggerController.SetupState setupState, boolean isReconnect, CountDownLatch startupLatch) {
+        Thread jdwpReceiver = new Thread(new JDWPReceiver(controller, setupState, isReconnect, startupLatch), "jdwp-receiver");
+        controller.addDebuggerReceiverThread(jdwpReceiver);
         jdwpReceiver.setDaemon(true);
         jdwpReceiver.start();
     }
 
-    static void reconnectDebuggerConnection(DebuggerController controller, DebuggerController.SetupState setupState) {
-        // On reconnect, we just pass a placeholder CountDownLatch object which we don't ever wait
-        // for. This avoids tedious null checks in the connection method.
-        establishDebuggerConnection(controller, setupState, true, new CountDownLatch(1));
+    public void dispose() {
+        connection.dispose();
     }
 
-    public void close() {
-        try {
-            connection.close(controller);
-        } catch (IOException e) {
-            throw new RuntimeException("Closing socket connection failed", e);
-        }
+    public void closeSocket() {
+        connection.closeSocket();
     }
 
     @Override
@@ -115,9 +109,7 @@ private static class JDWPSender implements Runnable {
 
         @Override
         public void run() {
-            while (!Thread.currentThread().isInterrupted()) {
-                socketConnection.awaitSendPacket();
-            }
+            socketConnection.sendPackets();
         }
     }
 
@@ -154,27 +146,50 @@ public void run() {
                 if (!HandshakeController.handshake(connectionSocket)) {
                     throw new IOException("Unable to handshake with debugger");
                 }
-                SocketConnection socketConnection = new SocketConnection(connectionSocket);
-                debuggerConnection = new DebuggerConnection(socketConnection, controller);
-                controller.setDebuggerConnection(debuggerConnection);
-                controller.getEventListener().setConnection(socketConnection);
-                if (!controller.isSuspend()) {
-                    // Fire the vm started event for the suspend=n case.
-                    // For suspend=y we have to synchronize the sending of VM started event with
-                    // the thread suspension count. Therefore, in that case we postpone the sending
-                    // until we can also suspend the main thread which is done in
-                    // DebuggerController#onLanguageContextInitialized.
-                    controller.getEventListener().vmStarted(false);
-                }
+                try {
+                    if (controller.isClosing()) {
+                        return;
+                    }
+                    // The following block has to be synchronized with resetting, so that
+                    // we can abandon further work in case we're told to tear down
+                    controller.getResettingLock().lockInterruptibly();
+                    // re-check to return immediately if closing
+                    if (controller.isClosing()) {
+                        return;
+                    }
+                    SocketConnection socketConnection = new SocketConnection(connectionSocket);
+                    debuggerConnection = new DebuggerConnection(socketConnection, controller);
+                    controller.setDebuggerConnection(debuggerConnection);
+                    controller.getEventListener().setConnection(socketConnection);
+                    if (!controller.isSuspend()) {
+                        // Fire the vm started event for the suspend=n case.
+                        // For suspend=y we have to synchronize the sending of VM started event with
+                        // the thread suspension count. Therefore, in that case we postpone the
+                        // sending until we can also suspend the main thread which is done in
+                        // DebuggerController#onLanguageContextInitialized.
+                        controller.getEventListener().vmStarted(false);
+                    }
 
-                // OK, we're ready to fire up the JDWP transmitter thread too
-                Thread jdwpSender = new Thread(new JDWPSender(socketConnection), "jdwp-transmitter");
-                controller.addDebuggerThread(jdwpSender);
-                jdwpSender.setDaemon(true);
-                jdwpSender.start();
+                    // OK, we're ready to fire up the JDWP transmitter thread too
+                    Thread jdwpSender = new Thread(new JDWPSender(socketConnection), "jdwp-transmitter");
+                    controller.addDebuggerSenderThread(jdwpSender);
+                    jdwpSender.setDaemon(true);
+                    jdwpSender.start();
+                } catch (InterruptedException e) {
+                    Thread.currentThread().interrupt();
+                    return;
+                } finally {
+                    controller.getResettingLock().unlock();
+                }
             } catch (IOException ex) {
                 if (isReconnect) {
-                    System.err.println("ERROR: Debuggers will not be able to connect to this context again!");
+                    // could be because we're closing down the context, so we should check that and
+                    // silently abort the re-connecting attempt
+                    if (controller.isClosing()) {
+                        return;
+                    } else {
+                        System.err.println("ERROR: Debuggers will not be able to connect to this context again!");
+                    }
                 } else {
                     // on startup any connection error is treated as fatal
                     controller.markLateStartupError(ex);
@@ -190,11 +205,15 @@ public void run() {
                     try {
                         processPacket(Packet.fromByteArray(debuggerConnection.connection.readPacket()));
                     } catch (IOException e) {
+                        if (!debuggerConnection.isOpen()) {
+                            // when the socket is closed, we're done
+                            break;
+                        }
                         if (!Thread.currentThread().isInterrupted()) {
                             controller.warning(() -> "Failed to process jdwp packet with message: " + e.getMessage());
                         }
                     } catch (ConnectionClosedException e) {
-                        // we closed the session, so let the thread run dry
+                        break;
                     }
                 }
             } finally {
@@ -244,7 +263,7 @@ private void processPacket(Packet packet) {
                                     result = JDWP.VirtualMachine.RESUME.createReply(packet, controller);
                                     break;
                                 case JDWP.VirtualMachine.EXIT.ID:
-                                    result = JDWP.VirtualMachine.EXIT.createReply(packet, context);
+                                    result = JDWP.VirtualMachine.EXIT.createReply(packet, context, controller);
                                     break;
                                 case JDWP.VirtualMachine.CREATE_STRING.ID:
                                     result = JDWP.VirtualMachine.CREATE_STRING.createReply(packet, context);

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/DebuggerController.java

@@ -33,6 +33,7 @@
 import java.util.Map;
 import java.util.concurrent.Callable;
 import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.locks.Lock;
 import java.util.function.Supplier;
 import java.util.logging.Level;
 import java.util.regex.Pattern;
@@ -131,11 +132,13 @@ public void reInitialize() {
         assert setupState != null;
 
         if (setupState.fatalConnectionError) {
-            System.err.println("ERROR: Debuggers will not be able to connect to this context again!");
+            fine(() -> "Failed debugger setup due to initial connection issue.");
             // OK, give up on trying to reconnect
             return;
         }
-        DebuggerConnection.reconnectDebuggerConnection(this, setupState);
+        // On reconnect, we just pass a placeholder CountDownLatch object which we don't ever wait
+        // for. This avoids tedious null checks in the connection method.
+        DebuggerConnection.establishDebuggerConnection(this, setupState, true, new CountDownLatch(1));
     }
 
     void setDebuggerConnection(DebuggerConnection connection) {
@@ -146,20 +149,38 @@ void setSetupState(SetupState state) {
         this.setupState = state;
     }
 
-    public void closeConnection() {
+    public void dispose() {
         if (connection != null) {
-            connection.close();
+            connection.dispose();
         }
     }
 
-    public void addDebuggerThread(Thread thread) {
-        instrument.addDebuggerThread(thread);
+    public void closeSocket() {
+        if (connection != null) {
+            connection.closeSocket();
+        }
+    }
+
+    public void addDebuggerReceiverThread(Thread thread) {
+        instrument.addDebuggerReceiverThread(thread);
+    }
+
+    public void addDebuggerSenderThread(Thread thread) {
+        instrument.addDebuggerSenderThread(thread);
     }
 
     public void markLateStartupError(Throwable t) {
         lateStartupError = t;
     }
 
+    public boolean isClosing() {
+        return instrument.isClosing();
+    }
+
+    public Lock getResettingLock() {
+        return instrument.getResettingLock();
+    }
+
     static final class SetupState {
         final Socket socket;
         final ServerSocket serverSocket;
@@ -505,15 +526,7 @@ public void disposeDebugger(boolean prepareReconnect) {
                 suspend(context.asGuestThread(Thread.currentThread()), SuspendStrategy.EVENT_THREAD, Collections.emptyList(), true);
             }
         }
-        // Creating a new thread, because the reset method
-        // will interrupt all active jdwp threads, which might
-        // include the current one if we received a DISPOSE command.
-        new Thread(new Runnable() {
-            @Override
-            public void run() {
-                instrument.reset(prepareReconnect);
-            }
-        }).start();
+        instrument.reset(prepareReconnect);
     }
 
     public void endSession() {

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/HandshakeController.java

@@ -51,7 +51,9 @@ void setupInitialConnection(DebuggerController controller) throws IOException {
         if (controller.isServer()) {
             ServerSocket serverSocket = new ServerSocket();
             serverSocket.setSoTimeout(0); // no timeout
-            serverSocket.setReuseAddress(true);
+            if (port != 0) {
+                serverSocket.setReuseAddress(true);
+            }
             if ("*".equals(controller.getHost())) {
                 // allow any host to bind
                 serverSocket.bind(new InetSocketAddress((InetAddress) null, port));

espresso/src/com.oracle.truffle.espresso.jdwp/src/com/oracle/truffle/espresso/jdwp/impl/JDWP.java

@@ -27,18 +27,17 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.concurrent.Callable;
 
 import com.oracle.truffle.api.interop.InteropException;
+import com.oracle.truffle.espresso.classfile.attributes.LineNumberTableRef;
+import com.oracle.truffle.espresso.classfile.attributes.LocalRef;
 import com.oracle.truffle.espresso.jdwp.api.CallFrame;
 import com.oracle.truffle.espresso.jdwp.api.ClassStatusConstants;
 import com.oracle.truffle.espresso.jdwp.api.ErrorCodes;
 import com.oracle.truffle.espresso.jdwp.api.FieldRef;
 import com.oracle.truffle.espresso.jdwp.api.JDWPConstantPool;
 import com.oracle.truffle.espresso.jdwp.api.JDWPContext;
 import com.oracle.truffle.espresso.jdwp.api.KlassRef;
-import com.oracle.truffle.espresso.classfile.attributes.LineNumberTableRef;
-import com.oracle.truffle.espresso.classfile.attributes.LocalRef;
 import com.oracle.truffle.espresso.jdwp.api.MethodRef;
 import com.oracle.truffle.espresso.jdwp.api.ModuleRef;
 import com.oracle.truffle.espresso.jdwp.api.MonitorStackInfo;
@@ -163,12 +162,9 @@ static class DISPOSE {
             static CommandResult createReply(Packet packet, DebuggerController controller) {
                 PacketStream reply = new PacketStream().replyPacket().id(packet.id);
 
-                return new CommandResult(reply, null, Collections.singletonList(new Callable<Void>() {
-                    @Override
-                    public Void call() throws Exception {
-                        controller.disposeDebugger(true);
-                        return null;
-                    }
+                return new CommandResult(reply, null, Collections.singletonList(() -> {
+                    controller.disposeDebugger(true);
+                    return null;
                 }));
             }
         }
@@ -214,19 +210,17 @@ static CommandResult createReply(Packet packet, DebuggerController controller) {
         static class EXIT {
             public static final int ID = 10;
 
-            static CommandResult createReply(Packet packet, JDWPContext context) {
+            static CommandResult createReply(Packet packet, JDWPContext context, DebuggerController controller) {
                 PacketStream input = new PacketStream(packet);
                 PacketStream reply = new PacketStream().replyPacket().id(packet.id);
 
                 if (context.systemExitImplemented()) {
                     return new CommandResult(reply,
                                     null,
-                                    Collections.singletonList(new Callable<Void>() {
-                                        @Override
-                                        public Void call() {
-                                            context.exit(input.readInt());
-                                            return null;
-                                        }
+                                    Collections.singletonList(() -> {
+                                        controller.disposeDebugger(false);
+                                        context.exit(input.readInt());
+                                        return null;
                                     }));
                 } else {
                     reply.errorCode(ErrorCodes.NOT_IMPLEMENTED);