implement async abstract methods, add TCP conn #91

wekesa360 · 2023-12-01T20:53:59Z

With this PR: the jdwp_streams.py adds the implementation of the two abstract classes JDWPOutputStreamBase and the JDWPInputStreamBase . With the jvm_connection.py has the JVMConnection class for connections to a JVM.

michalgr · 2023-12-04T10:14:15Z

projects/jdwp/jvm_connection.py

+        self.host = host
+        self.port = port
+        self._tcp_connection = None
+        self._input_stream = None
+        self._output_stream = None


Could you annotate attributes of the class ? (https://peps.python.org/pep-0526/#class-and-instance-variable-annotations)

Could you use __ instead of _ to prefix attributes that should not be accessed from outside of this class?

michalgr · 2023-12-04T10:17:27Z

projects/jdwp/jvm_connection.py

+            self._tcp_connection = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+            self._tcp_connection.connect((self.host, self.port))


We should be using asyncio.open_connection here: https://docs.python.org/3/library/asyncio-stream.html#asyncio.open_connection to create reader/writer pair that can be used to perform asynchronous IO.

michalgr · 2023-12-04T10:28:38Z

projects/jdwp/jvm_connection.py

+            await self._output_stream._write_bytes(handshake_bytes)
+
+            response_bytes = await self._input_stream._read_bytes(len(handshake_bytes))


The purpose of the JDWPInputStream and JDWPOutputStream is to allow us to express serialization and parsing of JDWP Structs in therms of the types defined in the spec. If there is a need for JVMConnection to send/receive anything else to/from the debugged JVM (and there is!) then it should use the underlying writer and reader returned from asyncio.open_connection directly.

michalgr · 2023-12-04T10:29:38Z

projects/jdwp/runtime/jdwp_streams.py

+class JDWPInputStream(JDWPInputStreamBase):
+    def __init__(self, socket_connection: socket.socket):
+        super().__init__()
+        self._tcp_connection: socket.socket = socket_connection


Should be asyncio.StreamReader. Also could you annotate it right at the top of the class definition ?

michalgr · 2023-12-04T10:46:11Z

projects/jdwp/runtime/jdwp_streams.py

+    async def read_packet_header(self) -> typing.Tuple[int, int, bytes, int, int]:
+        header_data = await self._read_bytes(10)
+        return struct.unpack("!IIcBB", header_data)
+
+    async def read_packet_data(self, length: int) -> bytes:
+        return await self._read_bytes(length)


These two should be moved elsewhere, maybe they should be private methods of the JVMConnection ? This class should focus on reading and writing JDWP values from the spec.

I think we will need a methods/functions with a signatures similar to:

async def send(command_set: int, command: int, struct: typing.Optional[JDWPStruct]): ... async def receive() -> JDWPStruct: ...

Those functions should handle packet headers and should use JDWPInputStream/JDWPOutputStreaum to turn structs into bytes.

One more thing, right now the receive method would have no idea what is the type of the response Struct it should be receiving (but it will in the future). Feel free to add an argument that is a class object indicating the type, something like:

JDWPStructT = TypeVar("TypeT", bound=JDWPStruct) async def receive(response: Optional[Type[JDWPStructT]] = None) -> JDWPStruct: ...

michalgr · 2023-12-04T10:49:39Z

projects/jdwp/runtime/jdwp_streams.py

+    async def _read_id(self, id_type: IdType) -> typing.Any:
+        size = await self._read_bytes(1)
+        size = struct.unpack("B", size)[0]
+        data = await self._read_bytes(size)
+        return getattr(IdType, id_type.value)(int.from_bytes(data, byteorder="big"))


This is not correct, is it ? The size is not prepended, we need to ask the VM for sizes with IDSizes command

michalgr · 2023-12-04T10:50:12Z

projects/jdwp/runtime/jdwp_streams.py

+    async def _read_type(self, type_id: IdType) -> typing.Any:
+        if type_id == IdType.BOOLEAN:
+            return await self.read_boolean()
+        elif type_id == IdType.INT:
+            return await self.read_int()
+        elif type_id == IdType.LONG:
+            return await self.read_long()
+        elif type_id == IdType.OBJECT_ID:
+            return await self.read_object_id()
+        elif type_id == IdType.THREAD_ID:
+            return await self.read_thread_id()
+        elif type_id == IdType.STRING:
+            return await self.read_string()
+        elif type_id == IdType.CLASS_ID:
+            return await self.read_reference_type_id()
+        else:
+            raise ValueError(f"Unsupported type ID: {type_id}")
+


This seems unused ?

michalgr · 2023-12-04T10:55:36Z

projects/jdwp/runtime/jdwp_streams.py

+    async def write_array(
+        self, element_type: IdType, elements: typing.List[typing.Any]
+    ) -> None:
+        await self.write_int(len(elements))
+
+        for element in elements:
+            await self._write_type(element_type, element)


Let's focus only on the abstract methods defined in the *Base class, this one is not there.

michalgr · 2023-12-04T11:00:20Z

projects/jdwp/jvm_connection.py

@@ -0,0 +1,48 @@
+# Copyright (c) Meta Platforms, Inc. and affiliates.


Could you move this file to projects/jdwp/runtime/jvm_connection.py ? You will need to make main depend on that target to make this code visible in main.py.

Codegen dataclassess (facebookexperimental#79) * Wrote test to validate primitive type mapping * Added test for type alias definition * Built dataclasses code generator * Built test for dataclassess code generator * Changed to unittest * Updated struct dataclass function to support all types * feat: built dataclass generator * Added copyright * Added nested struct support * Added test for nested struct * Feat: update functions and put it in a class. * chore: update tests * Casted types to pass pyre check * Updated types * chore: Refactor typing.cast code * Added copyright * refactor: update tests to reflect new schema changes * fix: Fix pyre typing errors [jdwp] do not emit array length and union tag fields [jdwp] map union tags and array lengths to int [jdwp] fix type for union fields [jdwp] fix names of fields containing spaces [jdwp] generate projects.jdwp.runtime.structs [jdwp] add tests importing components of the runtime library implement async abstract methods, add TCP conn update files update conn and streams add buck deps add jvm connection and implement jvm streams add a base class for jdwp structs [jdwp] generate projects.jdwp.runtime.structs [jdwp] add tests importing components of the runtime library fix fail build [jdwp] fix type checker fails

michalgr · 2023-12-04T11:13:34Z

projects/jdwp/runtime/jdwp_streams.py

+class JDWPOutputStream(JDWPOutputStreamBase):
+    def __init__(self, socket_connection: socket.socket):
+        super().__init__()
+        self._tcp_connection: socket.socket = socket_connection


I realised that we need to compute the size of the message and put it in the header, so we need to serialise structs before we write anything to the socket. Could you rename this to be JDWPBufferOutputStream and change it so that it appends all the data to an internal bytes object ? Then the JVMConnection.send method suggested above could look something like this:

async def send(command_set: int, command: int, struct: typing.Optional[JDWPStruct]): if struct is None: data = b"" else: struct_output_stream = JDWPBufferOutputStream() struct.serialize(struct_output_stream) data = JDWPBufferOutputStream.data # write header ... # write data self.writer.write(data)

michalgr · 2023-12-04T11:21:39Z

projects/jdwp/runtime/jdwp_streams.py

+    async def read_packet_header(self) -> typing.Tuple[int, int, bytes, int, int]:
+        header_data = await self._read_bytes(10)
+        return struct.unpack("!IIcBB", header_data)
+
+    async def read_packet_data(self, length: int) -> bytes:
+        return await self._read_bytes(length)


One more thing, right now the receive method would have no idea what is the type of the response Struct it should be receiving (but it will in the future). Feel free to add an argument that is a class object indicating the type, something like:

JDWPStructT = TypeVar("TypeT", bound=JDWPStruct) async def receive(response: Optional[Type[JDWPStructT]] = None) -> JDWPStruct: ...

michalgr · 2023-12-07T16:57:54Z

projects/jdwp/BUCK

 python_library(
    name = "lib",
    srcs = glob(["**/*.py"]),
    visibility = ["PUBLIC"],
+    deps = [ "//projects/jdwp/runtime:runtime"]
 )


lib target is obsolete now, could you just remove it and instead make main depend on //projects/jdwp/runtime:runtime directly ?

michalgr · 2023-12-07T17:35:26Z

projects/jdwp/runtime/async_streams.py

@@ -96,85 +96,85 @@ async def read_long(
 class JDWPOutputStreamBase(abc.ABC):
    # Methods for OpaqueType
    @abc.abstractmethod
-    def write_boolean(self, value: bool) -> None:
+    async def write_boolean(self, value: bool) -> None:


Output stream can (and should) be synchronous. StreamWriter.write is. Please remove changes in this file.

michalgr · 2023-12-07T17:36:37Z

projects/jdwp/runtime/jdwp_streams.py

+import struct
+import typing
+import asyncio
+from projects.jdwp.defs.schema import IdType


This probably does not need to be imported.

michalgr · 2023-12-07T17:54:15Z

projects/jdwp/runtime/jdwp_streams.py

+        pass
+
+
+class JDWPOutputStream(JDWPOutputStreamBase):


You missed my comment about JDWPBufferOutputStream. We need an output steam that appends everything to a byte buffer because we need to know the size of the entire message in order to write its header.

michalgr · 2023-12-07T18:01:25Z

projects/jdwp/runtime/jvm_connection.py

+        for stream in (self.__reader, self.__writer):
+            if stream is not None:
+                if isinstance(stream, asyncio.streams.StreamWriter):


It's a for loop with an if statement picking only one option. It seems like this logic could be greatly simplified ?

Yeah, thanks for the highlight, I had initially included a close() for the streamReader but looking at the asyncio docs there is no close() function for it.

michalgr · 2023-12-07T18:02:12Z

projects/jdwp/runtime/jvm_connection.py

+            print(f"Error during connection: {e}")
+            await self.close()
+
+    async def close(self) -> None:


Do we want this to be async ? There is not a single await in the body of the method

michalgr · 2023-12-07T18:02:34Z

projects/jdwp/runtime/jvm_connection.py

+    async def handshake(self) -> None:
+        try:
+            handshake_bytes = b"JDWP-Handshake"
+            if self.__output_stream is not None:


We should throw if this is not the case, right ?

michalgr · 2023-12-07T18:05:34Z

projects/jdwp/runtime/jvm_connection.py

+            print(f"Error during handshake: {e}")
+            await self.close()
+
+    async def read_packet_header(self) -> typing.Tuple[int, int, bytes, int, int]:


this should be private (__read...) and we need a NamedTuple to represent tuples with that many components:

class PacketHeader(typing.NamedTuple): length: int id: int ...

Once there is a named tuple representing packet header you can add a write and a static async read methods there.

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 1, 2023

michalgr requested changes Dec 4, 2023

View reviewed changes

wekesa360 force-pushed the jdwp_debugger_init branch 4 times, most recently from f7a5710 to 6beef3d Compare December 7, 2023 12:08

wekesa360 force-pushed the jdwp_debugger_init branch from 6beef3d to 4ce9a8d Compare December 7, 2023 12:38

michalgr requested changes Dec 7, 2023

View reviewed changes

wekesa360 added 2 commits December 8, 2023 17:13

[jdwp] fix jdwp_streams and connection

8e14a3b

[jdwp] update jvm streams and conn

737159d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement async abstract methods, add TCP conn #91

implement async abstract methods, add TCP conn #91

wekesa360 commented Dec 1, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 4, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

wekesa360 Dec 8, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

michalgr Dec 7, 2023

		self._tcp_connection = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
		self._tcp_connection.connect((self.host, self.port))

		await self._output_stream._write_bytes(handshake_bytes)

		response_bytes = await self._input_stream._read_bytes(len(handshake_bytes))

		@@ -0,0 +1,48 @@
		# Copyright (c) Meta Platforms, Inc. and affiliates.

implement async abstract methods, add TCP conn #91

Are you sure you want to change the base?

implement async abstract methods, add TCP conn #91

Conversation

wekesa360 commented Dec 1, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment