Skip to content

Commit 4116996

Browse files
authored
FIX: Robust resource cleanup of Connection/Cursor for graceful exit & add tests (#128)
### ADO Work Item Reference <!-- Insert your ADO Work Item ID below (e.g. AB#37452) --> > [AB#37889](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/37889) > Github Issue - #131 ------------------------------------------------------------------- ### Summary <!-- Insert your Copilot Generated Summary below --> This pull request introduces enhancements to cursor and connection management in the `mssql_python` library, aiming to improve resource cleanup and debugging capabilities. Key changes include adding destructors for automatic cleanup, enhancing logging for better traceability, and introducing new tests to validate cursor behavior during garbage collection and connection closure. ### Resource Cleanup Enhancements: * Added a `__del__` method in `mssql_python/connection.py` to automatically close connections during object destruction, with error logging for cleanup issues. * Added a `__del__` method in `mssql_python/cursor.py` to ensure cursors are closed when no longer needed, with error handling during destruction. ### Debugging Improvements: * Enhanced logging in `mssql_python/connection.py` and `mssql_python/cursor.py` with detailed print statements for tracking cursor states and operations (e.g., during connection closure, statement execution, and data fetching). [[1]](diffhunk://#diff-29bb94de45aae51c23a6426d40133c28e4161e68769e08d046059c7186264e90R227-R244) [[2]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280R426-R435) [[3]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280R565-R566) [[4]](diffhunk://#diff-deceea46ae01082ce8400e14fa02f4b7585afb7b5ed9885338b66494f5f38280R715-L709) ### Testing Additions: * Introduced `test_cursor_cleanup_without_close` to verify that cursors are properly cleaned up when the connection is not explicitly closed. * Added `test_no_segfault_on_gc` to ensure no segmentation faults occur during garbage collection of connections and cursors. This test runs in a subprocess to isolate potential crashes. <!-- ### PR Title Guide > For feature requests FEAT: (short-description) > For non-feature requests like test case updates, config updates , dependency updates etc CHORE: (short-description) > For Fix requests FIX: (short-description) > For doc update requests DOC: (short-description) > For Formatting, indentation, or styling update STYLE: (short-description) > For Refactor, without any feature changes REFACTOR: (short-description) > For release related changes, without any feature changes RELEASE: #<RELEASE_VERSION> (short-description) -->
1 parent ef70493 commit 4116996

File tree

6 files changed

+308
-178
lines changed

6 files changed

+308
-178
lines changed

mssql_python/connection.py

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,6 @@ def cursor(self) -> Cursor:
176176
)
177177

178178
cursor = Cursor(self)
179-
self._cursors.add(cursor) # Track the cursor
180179
return cursor
181180

182181
def commit(self) -> None:
@@ -234,7 +233,7 @@ def close(self) -> None:
234233
# Convert to list to avoid modification during iteration
235234
cursors_to_close = list(self._cursors)
236235
close_errors = []
237-
236+
238237
for cursor in cursors_to_close:
239238
try:
240239
if not cursor.closed:
@@ -268,3 +267,16 @@ def close(self) -> None:
268267

269268
if ENABLE_LOGGING:
270269
logger.info("Connection closed successfully.")
270+
271+
def __del__(self):
272+
"""
273+
Destructor to ensure the connection is closed when the connection object is no longer needed.
274+
This is a safety net to ensure resources are cleaned up
275+
even if close() was not called explicitly.
276+
"""
277+
if not self._closed:
278+
try:
279+
self.close()
280+
except Exception as e:
281+
if ENABLE_LOGGING:
282+
logger.error(f"Error during connection cleanup in __del__: {e}")

mssql_python/cursor.py

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -416,23 +416,36 @@ def _initialize_cursor(self) -> None:
416416
"""
417417
Initialize the DDBC statement handle.
418418
"""
419+
# Allocate the DDBC statement handle
419420
self._allocate_statement_handle()
421+
# Add the cursor to the connection's cursor set
422+
self.connection._cursors.add(self)
420423

421424
def _allocate_statement_handle(self):
422425
"""
423426
Allocate the DDBC statement handle.
424427
"""
425428
self.hstmt = self.connection._conn.alloc_statement_handle()
426429

427-
def _reset_cursor(self) -> None:
430+
def _free_cursor(self) -> None:
428431
"""
429-
Reset the DDBC statement handle.
432+
Free the DDBC statement handle and remove the cursor from the connection's cursor set.
430433
"""
431434
if self.hstmt:
432435
self.hstmt.free()
433436
self.hstmt = None
434437
if ENABLE_LOGGING:
435-
logger.debug("SQLFreeHandle succeeded")
438+
logger.debug("SQLFreeHandle succeeded")
439+
# We don't need to remove the cursor from the connection's cursor set here,
440+
# as it is a weak reference and will be automatically removed
441+
# when the cursor is garbage collected.
442+
443+
def _reset_cursor(self) -> None:
444+
"""
445+
Reset the DDBC statement handle.
446+
"""
447+
# Free the current cursor if it exists
448+
self._free_cursor()
436449
# Reinitialize the statement handle
437450
self._initialize_cursor()
438451

@@ -706,7 +719,6 @@ def fetchall(self) -> List[Row]:
706719
# Fetch raw data
707720
rows_data = []
708721
ret = ddbc_bindings.DDBCSQLFetchAll(self.hstmt, rows_data)
709-
710722
# Convert raw data to Row objects
711723
return [Row(row_data, self.description) for row_data in rows_data]
712724

@@ -727,4 +739,16 @@ def nextset(self) -> Union[bool, None]:
727739
check_error(ddbc_sql_const.SQL_HANDLE_STMT.value, self.hstmt, ret)
728740
if ret == ddbc_sql_const.SQL_NO_DATA.value:
729741
return False
730-
return True
742+
return True
743+
744+
def __del__(self):
745+
"""
746+
Destructor to ensure the cursor is closed when it is no longer needed.
747+
This is a safety net to ensure resources are cleaned up
748+
even if close() was not called explicitly.
749+
"""
750+
if not self.closed:
751+
try:
752+
self.close()
753+
except Exception as e:
754+
logger.error(f"Error closing cursor: {e}")

tests/test_003_connection.py

Lines changed: 1 addition & 171 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,10 @@
1010
Note: The cursor function is not yet implemented, so related tests are commented out.
1111
"""
1212

13-
from mssql_python.exceptions import InterfaceError
1413
import pytest
1514
import time
1615
from mssql_python import Connection, connect, pooling
17-
16+
1817
def drop_table_if_exists(cursor, table_name):
1918
"""Drop the table if it exists"""
2019
try:
@@ -224,172 +223,3 @@ def test_connection_pooling_basic(conn_str):
224223

225224
conn1.close()
226225
conn2.close()
227-
228-
# Add these tests at the end of the file
229-
230-
def test_cursor_cleanup_on_connection_close(conn_str):
231-
"""Test that cursors are properly cleaned up when connection is closed"""
232-
# Create a new connection for this test
233-
conn = connect(conn_str)
234-
235-
# Create multiple cursors
236-
cursor1 = conn.cursor()
237-
cursor2 = conn.cursor()
238-
cursor3 = conn.cursor()
239-
240-
# Execute something on each cursor to ensure they have statement handles
241-
# Option 1: Fetch results immediately to free the connection
242-
cursor1.execute("SELECT 1")
243-
cursor1.fetchall()
244-
245-
cursor2.execute("SELECT 2")
246-
cursor2.fetchall()
247-
248-
cursor3.execute("SELECT 3")
249-
cursor3.fetchall()
250-
251-
# Close one cursor explicitly
252-
cursor1.close()
253-
assert cursor1.closed is True, "Cursor1 should be closed"
254-
255-
# Close the connection (should clean up remaining cursors)
256-
conn.close()
257-
258-
# Verify all cursors are closed
259-
assert cursor1.closed is True, "Cursor1 should remain closed"
260-
assert cursor2.closed is True, "Cursor2 should be closed by connection.close()"
261-
assert cursor3.closed is True, "Cursor3 should be closed by connection.close()"
262-
263-
def test_cursor_weakref_cleanup(conn_str):
264-
"""Test that WeakSet properly removes garbage collected cursors"""
265-
conn = connect(conn_str)
266-
267-
# Create cursors
268-
cursor1 = conn.cursor()
269-
cursor2 = conn.cursor()
270-
271-
# Check initial cursor count
272-
assert len(conn._cursors) == 2, "Should have 2 cursors"
273-
274-
# Delete reference to cursor1 (should be garbage collected)
275-
cursor1_id = id(cursor1)
276-
del cursor1
277-
278-
# Force garbage collection
279-
import gc
280-
gc.collect()
281-
282-
# Check cursor count after garbage collection
283-
assert len(conn._cursors) == 1, "Should have 1 cursor after garbage collection"
284-
285-
# Verify cursor2 is still there
286-
assert cursor2 in conn._cursors, "Cursor2 should still be in the set"
287-
288-
conn.close()
289-
290-
def test_cursor_cleanup_order_no_segfault(conn_str):
291-
"""Test that proper cleanup order prevents segfaults"""
292-
# This test ensures cursors are cleaned before connection
293-
conn = connect(conn_str)
294-
295-
# Create multiple cursors with active statements
296-
cursors = []
297-
for i in range(5):
298-
cursor = conn.cursor()
299-
cursor.execute(f"SELECT {i}")
300-
cursor.fetchall()
301-
cursors.append(cursor)
302-
303-
# Don't close any cursors explicitly
304-
# Just close the connection - it should handle cleanup properly
305-
conn.close()
306-
307-
# Verify all cursors were closed
308-
for cursor in cursors:
309-
assert cursor.closed is True, "All cursors should be closed"
310-
311-
def test_cursor_close_removes_from_connection(conn_str):
312-
"""Test that closing a cursor properly cleans up references"""
313-
conn = connect(conn_str)
314-
315-
# Create cursors
316-
cursor1 = conn.cursor()
317-
cursor2 = conn.cursor()
318-
cursor3 = conn.cursor()
319-
320-
assert len(conn._cursors) == 3, "Should have 3 cursors"
321-
322-
# Close cursor2
323-
cursor2.close()
324-
325-
# cursor2 should still be in the WeakSet (until garbage collected)
326-
# but it should be marked as closed
327-
assert cursor2.closed is True, "Cursor2 should be closed"
328-
329-
# Delete the reference and force garbage collection
330-
del cursor2
331-
import gc
332-
gc.collect()
333-
334-
# Now should have 2 cursors
335-
assert len(conn._cursors) == 2, "Should have 2 cursors after closing and GC"
336-
337-
conn.close()
338-
339-
def test_connection_close_idempotent(conn_str):
340-
"""Test that calling close() multiple times is safe"""
341-
conn = connect(conn_str)
342-
cursor = conn.cursor()
343-
cursor.execute("SELECT 1")
344-
345-
# First close
346-
conn.close()
347-
assert conn._closed is True, "Connection should be closed"
348-
349-
# Second close (should not raise exception)
350-
conn.close()
351-
assert conn._closed is True, "Connection should remain closed"
352-
353-
# Cursor should also be closed
354-
assert cursor.closed is True, "Cursor should be closed"
355-
356-
def test_cursor_after_connection_close(conn_str):
357-
"""Test that creating cursor after connection close raises error"""
358-
conn = connect(conn_str)
359-
conn.close()
360-
361-
# Should raise exception when trying to create cursor on closed connection
362-
with pytest.raises(InterfaceError) as excinfo:
363-
cursor = conn.cursor()
364-
365-
assert "closed connection" in str(excinfo.value).lower(), "Should mention closed connection"
366-
367-
def test_multiple_cursor_operations_cleanup(conn_str):
368-
"""Test cleanup with multiple cursor operations"""
369-
conn = connect(conn_str)
370-
371-
# Create table for testing
372-
cursor_setup = conn.cursor()
373-
drop_table_if_exists(cursor_setup, "#test_cleanup")
374-
cursor_setup.execute("CREATE TABLE #test_cleanup (id INT, value VARCHAR(50))")
375-
cursor_setup.close()
376-
377-
# Create multiple cursors doing different operations
378-
cursor_insert = conn.cursor()
379-
cursor_insert.execute("INSERT INTO #test_cleanup VALUES (1, 'test1'), (2, 'test2')")
380-
381-
cursor_select1 = conn.cursor()
382-
cursor_select1.execute("SELECT * FROM #test_cleanup WHERE id = 1")
383-
cursor_select1.fetchall()
384-
385-
cursor_select2 = conn.cursor()
386-
cursor_select2.execute("SELECT * FROM #test_cleanup WHERE id = 2")
387-
cursor_select2.fetchall()
388-
389-
# Close connection without closing cursors
390-
conn.close()
391-
392-
# All cursors should be closed
393-
assert cursor_insert.closed is True
394-
assert cursor_select1.closed is True
395-
assert cursor_select2.closed is True

0 commit comments

Comments
 (0)