Browse Source

Add cleanup for Protocol objects to fix resource warnings

- Add __del__ method to Protocol that raises ResourceWarning if not explicitly closed
- Fix test_client tests to properly close Protocol objects returned by _connect()
- Fix TestSSHVendor.Subprocess mock methods to accept proper arguments
Jelmer Vernooij 3 weeks ago
parent
commit
f996e54f56
2 changed files with 43 additions and 12 deletions
  1. 18 0
      dulwich/protocol.py
  2. 25 12
      tests/test_client.py

+ 18 - 0
dulwich/protocol.py

@@ -401,6 +401,24 @@ class Protocol:
         """Close the underlying transport if a close function was provided."""
         if self._close:
             self._close()
+            self._close = None  # Prevent double-close
+
+    def __del__(self) -> None:
+        """Ensure transport is closed when Protocol is garbage collected."""
+        if self._close is not None:
+            import warnings
+
+            warnings.warn(
+                f"unclosed Protocol {self!r}",
+                ResourceWarning,
+                stacklevel=2,
+                source=self,
+            )
+            try:
+                self.close()
+            except Exception:
+                # Ignore errors during cleanup
+                pass
 
     def __enter__(self) -> "Protocol":
         """Enter context manager."""

+ 25 - 12
tests/test_client.py

@@ -933,12 +933,18 @@ class TestSSHVendor:
         self.protocol_version = protocol_version
 
         class Subprocess:
-            pass
+            def read(self, *args):
+                return None
+
+            def write(self, *args):
+                return None
+
+            def close(self):
+                pass
+
+            def can_read(self):
+                return None
 
-        Subprocess.read = lambda: None
-        Subprocess.write = lambda: None
-        Subprocess.close = lambda: None
-        Subprocess.can_read = lambda: None
         return Subprocess()
 
 
@@ -996,13 +1002,19 @@ class SSHGitClientTests(TestCase):
         client.username = b"username"
         client.port = 1337
 
-        client._connect(b"command", b"/path/to/repo")
-        self.assertEqual(b"username", server.username)
-        self.assertEqual(1337, server.port)
-        self.assertEqual(b"git-command '/path/to/repo'", server.command)
+        proto, _, _ = client._connect(b"command", b"/path/to/repo")
+        try:
+            self.assertEqual(b"username", server.username)
+            self.assertEqual(1337, server.port)
+            self.assertEqual(b"git-command '/path/to/repo'", server.command)
+        finally:
+            proto.close()
 
-        client._connect(b"relative-command", b"/~/path/to/repo")
-        self.assertEqual(b"git-relative-command '~/path/to/repo'", server.command)
+        proto, _, _ = client._connect(b"relative-command", b"/~/path/to/repo")
+        try:
+            self.assertEqual(b"git-relative-command '~/path/to/repo'", server.command)
+        finally:
+            proto.close()
 
     def test_ssh_command_precedence(self) -> None:
         self.overrideEnv("GIT_SSH", "/path/to/ssh")
@@ -1053,7 +1065,8 @@ class SSHGitClientTests(TestCase):
         client.key_filename = "/path/to/key"
 
         # Connect and verify all kwargs are passed through
-        client._connect(b"upload-pack", b"/path/to/repo")
+        proto, _, _ = client._connect(b"upload-pack", b"/path/to/repo")
+        self.addCleanup(proto.close)
 
         self.assertEqual(server.ssh_command, "custom-ssh-wrapper.sh -o Option=Value")
         self.assertEqual(server.password, "test-password")