Skip to content

Commit c7901ab

Browse files
ai: apply changes for #873 (2 review threads)
Addresses: - #3635836346 at src/databricks/sql/client.py:1764 - #3635836348 at src/databricks/sql/client.py:1770 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
1 parent e3658da commit c7901ab

2 files changed

Lines changed: 64 additions & 1 deletion

File tree

src/databricks/sql/client.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1761,11 +1761,13 @@ def close(self) -> None:
17611761
self.open = False
17621762
if self.active_result_set:
17631763
self._close_and_clear_active_result_set()
1764-
elif self.active_command_id is not None:
1764+
elif self.active_command_id is not None and self.connection.open:
17651765
# Async submission whose result was never fetched (no
17661766
# get_execution_result call), so the result-set close path never
17671767
# fired. Issue an explicit close_command to free the server-side
17681768
# statement handle instead of leaking it until session close.
1769+
# Gate on connection.open (mirroring ResultSet.close) so we don't
1770+
# attempt a network call on an already-torn-down session.
17691771
try:
17701772
self.backend.close_command(self.active_command_id)
17711773
except Exception as exc:

tests/unit/test_client.py

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -161,6 +161,67 @@ def test_closing_connection_closes_commands(self, mock_thrift_client_class):
161161
# Should NOT have called backend.close_command (already closed)
162162
mock_backend.close_command.assert_not_called()
163163

164+
def test_cursor_close_frees_command_when_no_result_set(self):
165+
"""Closing a cursor with an unfetched async command frees the server handle.
166+
167+
When active_result_set is None but active_command_id is set (an async
168+
submission whose result was never fetched), close() must issue an
169+
explicit backend.close_command so the server-side statement handle is
170+
not leaked. This branch is backend-agnostic (Thrift/SEA/kernel).
171+
"""
172+
mock_backend = Mock(spec=ThriftDatabricksClient)
173+
mock_connection = Mock()
174+
cursor = client.Cursor(connection=mock_connection, backend=mock_backend)
175+
176+
command_id = Mock(spec=CommandId)
177+
cursor.active_command_id = command_id
178+
cursor.active_result_set = None
179+
180+
cursor.close()
181+
182+
mock_backend.close_command.assert_called_once_with(command_id)
183+
self.assertIsNone(cursor.active_command_id)
184+
185+
def test_cursor_close_skips_command_when_connection_closed(self):
186+
"""Closing a cursor after its session is gone must not call close_command.
187+
188+
Mirrors ResultSet.close, which gates backend.close_command on
189+
connection.open, to avoid a network call on a dead session (and a
190+
spurious warning) during shutdown ordering.
191+
"""
192+
mock_backend = Mock(spec=ThriftDatabricksClient)
193+
mock_connection = Mock()
194+
mock_connection.open = False
195+
cursor = client.Cursor(connection=mock_connection, backend=mock_backend)
196+
197+
cursor.active_command_id = Mock(spec=CommandId)
198+
cursor.active_result_set = None
199+
200+
cursor.close()
201+
202+
mock_backend.close_command.assert_not_called()
203+
self.assertIsNone(cursor.active_command_id)
204+
205+
def test_cursor_close_does_not_double_close_when_result_set_present(self):
206+
"""Closing a cursor with an active result set must NOT call close_command.
207+
208+
The result-set close path is responsible for freeing the handle in that
209+
case, so the elif branch must not fire (no double-close).
210+
"""
211+
mock_backend = Mock(spec=ThriftDatabricksClient)
212+
mock_connection = Mock()
213+
cursor = client.Cursor(connection=mock_connection, backend=mock_backend)
214+
215+
cursor.active_command_id = Mock(spec=CommandId)
216+
result_set = Mock(spec=ResultSet)
217+
cursor.active_result_set = result_set
218+
219+
cursor.close()
220+
221+
result_set.close.assert_called_once()
222+
mock_backend.close_command.assert_not_called()
223+
self.assertIsNone(cursor.active_command_id)
224+
164225
@patch("%s.session.ThriftDatabricksClient" % PACKAGE_NAME)
165226
def test_cant_open_cursor_on_closed_connection(self, mock_client_class):
166227
connection = databricks.sql.connect(**self.DUMMY_CONNECTION_ARGS)

0 commit comments

Comments
 (0)