[lldb/test] Add ability to terminate connection from a gdb-client handler

[labath]

Summary

We were using the client socket close as a way to terminate the handler
thread. But this kind of concurrent access to the same socket is not
safe. It also complicates running the handler without a dedicated thread
(next patch).

Instead, here I add an explicit way for a packet handler to request
termination. Waiting for lldb to terminate the connection would almost
be sufficient, but in the pty test we want to keep the pty open so we
can examine its state. Ability to disconnect at an arbitrary point may
be useful for testing other aspects of lldb functionality as well.

The way this works is that now each packet handler can optionally return
a list of responses (instead of just one). One of those responses (it
only makes sense for it to be the last one) can be a special
RESPONSE_DISCONNECT object, which triggers a disconnection (via a new
TerminateConnectionException).

As the mock server now cleans up the connection whenever it disconnects,
the pty test needs to explicitly dup(2) the descriptors in order to
inspect the post-disconnect state.

[mgorny]

lldb/packages/Python/lldbsuite/test/gdbclientutils.py

89

How about making it a class instead? It should make debugging easier, if it ever comes to that.

In [6]: RESPONSE_DISCONNECT = object()

In [7]: RESPONSE_DISCONNECT
Out[7]: <object at 0x7f406dd328e0>

In [8]: class RESPONSE_DISCONNECT: pass

In [9]: RESPONSE_DISCONNECT
Out[9]: __main__.RESPONSE_DISCONNECT

611

It feels weird that you're raising class rather than an object.

[labath]

lldb/packages/Python/lldbsuite/test/gdbclientutils.py

89

I wouldn't call myself an expert on idiomatic python, but I was under the impression this is how one is expected to do these things https://python-patterns.guide/python/sentinel-object/ (?)

611

It looks like the class gets automatically instantiated (https://docs.python.org/3/tutorial/errors.html#raising-exceptions), though now that I think about it, I am actually surprised that it does :P

[labath]

Don't try to raise classes

[mgorny]

lldb/packages/Python/lldbsuite/test/gdbclientutils.py

89

Well, I see both versions in CPython stdlib but the advantage of using a class is that it gives clear repr() rather than identifying as an arbitrary object. You can also create an object of the sentinel class ;-).

[labath]

use a class sentinel

[mgorny]

LGTM

lldb/packages/Python/lldbsuite/test/gdbclientutils.py

class MockGDBServerResponder:
     completely customize their responses.
 
     Most subclasses will be interested in overriding the other() method, which
     handles any packet not recognized in the common packet handling code.
     """
 
     registerCount = 40
     packetLog = None
+    class RESPONSE_DISCONNECT: pass
 
     def __init__(self):
         self.packetLog = []
 
     def respond(self, packet):
         """
         Return the unframed packet data that the server should issue in response
         to the given packet received from the client.
class MockGDBServerResponder:
 
     def QEnvironmentHexEncoded(self, packet):
         return "OK"
 
     def qRegisterInfo(self, num):
         return ""
 
     def k(self):
-        return ""
+        return ["W01", self.RESPONSE_DISCONNECT]
 
     """
     Raised when we receive a packet for which there is no default action.
     Override the responder class to implement behavior suitable for the test at
     hand.
     """
     class UnexpectedPacketException(Exception):
         pass
def _run(self):
         try:
             self._socket.accept()
         except:
             return
         self._shouldSendAck = True
         self._receivedData = ""
         self._receivedDataOffset = 0
         data = None
-        while True:
-            try:
+            while True:
                 data = seven.bitcast_to_string(self._socket.recv())
                 if data is None or len(data) == 0:
                     break
                 self._receive(data)
-            except Exception as e:
-                print("An exception happened when receiving the response from the gdb server. Closing the client...")
-                traceback.print_exc()
-                self._socket.close_connection()
-                break
+        except self.TerminateConnectionException:
+            pass
+        finally:
+            self._socket.close_server()
 
     def _receive(self, data):
         """
         Collects data, parses and responds to as many packets as exist.
         Any leftover data is kept for parsing the next time around.
         """
         self._receivedData += data
-        try:
-            packet = self._parsePacket()
-            while packet is not None:
-                self._handlePacket(packet)
-                packet = self._parsePacket()
-        except self.InvalidPacketException:
-            self._socket.close_connection()
 
     def _parsePacket(self):
         """
         Reads bytes from self._receivedData, returning:
         - a packet's contents if a valid packet is found
         - the PACKET_ACK unique object if we got an ack
         - None if we only have a partial packet
 
def _parsePacket(self):
                     "Checksum %02x does not match content %02x" %
                     (check, checksum(packet)))
         # remove parsed bytes from _receivedData and reset offset so parsing
         # can start on the next packet the next time around
         self._receivedData = data[i:]
         self._receivedDataOffset = 0
         return packet
 
+    def _sendPacket(self, packet):
+        self._socket.sendall(seven.bitcast_to_bytes(frame_packet(packet)))
+
     def _handlePacket(self, packet):
         if packet is self.PACKET_ACK:
             # Ignore ACKs from the client. For the future, we can consider
             # adding validation code to make sure the client only sends ACKs
             # when it's supposed to.
             return
         response = ""
         # We'll handle the ack stuff here since it's not something any of the
         # tests will be concerned about, and it'll get turned off quickly anyway.
         if self._shouldSendAck:
             self._socket.sendall(seven.bitcast_to_bytes('+'))
         if packet == "QStartNoAckMode":
             self._shouldSendAck = False
             response = "OK"
         elif self.responder is not None:
             # Delegate everything else to our responder
             response = self.responder.respond(packet)
-        # Handle packet framing since we don't want to bother tests with it.
-        if response is not None:
-            framed = frame_packet(response)
-            self._socket.sendall(seven.bitcast_to_bytes(framed))
+        if not isinstance(response, list):
+            response = [response]
+        for part in response:
+            if part is MockGDBServerResponder.RESPONSE_DISCONNECT:
+                raise self.TerminateConnectionException()
+            self._sendPacket(part)
 
     PACKET_ACK = object()
     PACKET_INTERRUPT = object()
 
-    class InvalidPacketException(Exception):
+    class TerminateConnectionException(Exception):
         pass
 
+    class InvalidPacketException(Exception):
+        pass

lldb/test/API/functionalities/gdb_remote_client/TestPty.py

 import lldb
 from lldbsuite.test.lldbtest import *
 from lldbsuite.test.decorators import *
 from lldbsuite.test.gdbclientutils import *
 from lldbsuite.test.lldbgdbclient import GDBRemoteTestBase
 
 
 @skipIfWindows
 class TestPty(GDBRemoteTestBase):
     mydir = TestBase.compute_mydir(__file__)
     server_socket_class = PtyServerSocket
 
     def get_term_attrs(self):
         import termios
-        return termios.tcgetattr(self.server._socket._secondary)
+        return termios.tcgetattr(self._secondary_socket)
 
     def setUp(self):
         super().setUp()
+        # Duplicate the pty descriptors so we can inspect the pty state after
+        # they are closed
+        self._primary_socket = os.dup(self.server._socket._primary.name)
+        self._secondary_socket = os.dup(self.server._socket._secondary.name)
         self.orig_attr = self.get_term_attrs()
 
     def assert_raw_mode(self, current_attr):
         import termios
         self.assertEqual(current_attr[0] & (termios.BRKINT |
                                             termios.PARMRK |
                                             termios.ISTRIP | termios.INLCR |
                                             termios.IGNCR | termios.ICRNL |