Session is not set in object if connection fails, which raises 'AttributeError' on __del__ (#746)

src/databricks/sql/client.py

@@ -462,6 +462,12 @@ def __exit__(self, exc_type, exc_value, traceback):
         self.close()
 
     def __del__(self):
+        # If construction failed before ``self.session`` was assigned (e.g. the
+        # Session constructor raised), there is nothing to close. Guard against
+        # the missing attribute so __del__ doesn't raise during garbage
+        # collection. See https://github.com/databricks/databricks-sql-python/issues/746
+        if not hasattr(self, "session"):
+            return
         if self.open:
             logger.debug(
                 "Closing unclosed connection for session "

tests/e2e/test_driver.py

@@ -2,6 +2,7 @@
 from contextlib import contextmanager
 from collections import OrderedDict
 import datetime
+import gc
 import io
 import logging
 import os
@@ -1155,6 +1156,45 @@ def test_row_limit_with_arrow_smaller_result(self):
 
 # use a RetrySuite to encapsulate these tests which we'll typically want to run together; however keep
 # the 429/503 subsuites separate since they execute under different circumstances.
+class TestPySQLConnectionFailureSuite(PySQLPytestTestCase):
+    """Regression tests for connection construction failures (issue #746)."""
+
+    def test_del_does_not_raise_when_session_never_set(self):
+        """A constructor-stage connection failure must not leave __del__ raising
+        AttributeError when the half-constructed Connection is garbage-collected.
+
+        Username/password auth raises ValueError inside the Session constructor,
+        i.e. before ``self.session`` is assigned in Connection.__init__. The
+        original connection error must propagate, and __del__ on the
+        half-constructed Connection must not raise (which Python would report as
+        an 'Exception ignored in __del__' referencing the missing ``session``
+        attribute).
+        """
+        # Use the live-warehouse connection params, but inject username/password
+        # which triggers a client-side ValueError inside Session.__init__.
+        params = dict(self.connection_params(), username="user", password="pass")
+
+        unraisable = []
+        original_hook = sys.unraisablehook
+        sys.unraisablehook = lambda unraisable_hook_args: unraisable.append(
+            unraisable_hook_args
+        )
+        try:
+            with pytest.raises(ValueError):
+                sql.connect(**params)
+
+            # Force collection of the half-constructed Connection so __del__ runs.
+            gc.collect()
+        finally:
+            sys.unraisablehook = original_hook
+
+        messages = [
+            "{}: {}".format(type(u.exc_value).__name__, u.exc_value)
+            for u in unraisable
+        ]
+        assert not unraisable, "Exception(s) raised in __del__: {}".format(messages)
+
+
 class TestPySQLRetrySuite:
     class HTTP429Suite(Client429ResponseMixin, PySQLPytestTestCase):
         pass  # Mixin covers all

tests/unit/test_client.py

@@ -90,6 +90,38 @@ class ClientTestSuite(unittest.TestCase):
         "enable_telemetry": False,
     }
 
+    @patch("%s.client.Session" % PACKAGE_NAME)
+    def test_del_does_not_raise_when_session_never_set(self, mock_session_class):
+        """A constructor-stage failure must not leave __del__ raising on GC.
+
+        When the Session constructor raises before ``self.session`` is assigned
+        in Connection.__init__, the original error must propagate, and __del__ on
+        the half-constructed Connection must not raise (which Python reports as an
+        'Exception ignored in __del__' referencing the missing ``session``
+        attribute). See https://github.com/databricks/databricks-sql-python/issues/746
+        """
+        mock_session_class.side_effect = ValueError("bad auth")
+
+        unraisable = []
+        original_hook = sys.unraisablehook
+        sys.unraisablehook = lambda args: unraisable.append(args)
+        try:
+            with self.assertRaises(ValueError):
+                client.Connection(**self.DUMMY_CONNECTION_ARGS)
+
+            # Force collection of the half-constructed Connection so __del__ runs.
+            gc.collect()
+        finally:
+            sys.unraisablehook = original_hook
+
+        messages = [
+            "{}: {}".format(type(u.exc_value).__name__, u.exc_value)
+            for u in unraisable
+        ]
+        self.assertEqual(
+            unraisable, [], "Exception(s) raised in __del__: {}".format(messages)
+        )
+
     @patch("%s.session.ThriftDatabricksClient" % PACKAGE_NAME)
     def test_closing_connection_closes_commands(self, mock_thrift_client_class):
         """Test that closing a connection properly closes commands.