diff --git a/src/databricks/sql/client.py b/src/databricks/sql/client.py index d45d51181..8b4339a8c 100755 --- a/src/databricks/sql/client.py +++ b/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 " diff --git a/tests/e2e/test_driver.py b/tests/e2e/test_driver.py index 5fe3db037..9694ef8cf 100644 --- a/tests/e2e/test_driver.py +++ b/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 diff --git a/tests/unit/test_client.py b/tests/unit/test_client.py index 66a3722ec..7f938bfa1 100644 --- a/tests/unit/test_client.py +++ b/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.