Skip to content

Commit a9632fb

Browse files
authored
FIX: Ensure cursors are freed before connection (segfault) with added tests (#123)
### ADO Work Item Reference <!-- Insert your ADO Work Item ID below (e.g. AB#37452) --> > [AB#37863](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/37863) ------------------------------------------------------------------- ### Summary <!-- Insert your Copilot Generated Summary below --> This pull request introduces several updates to enhance connection management, improve cursor handling, and update test cases for better coverage and consistency. The most important changes include the implementation of weak references for cursor tracking, additional safeguards for connection cleanup, and modifications to test cases to align with schema conventions. ### Connection Management Enhancements: * Replaced `ctypes` with `weakref` and introduced `WeakSet` for managing cursors to prevent memory leaks and ensure automatic cleanup of unused cursors. (`mssql_python/connection.py`) [[1]](diffhunk://#diff-29bb94de45aae51c23a6426d40133c28e4161e68769e08d046059c7186264e90L8-R8) [[2]](diffhunk://#diff-29bb94de45aae51c23a6426d40133c28e4161e68769e08d046059c7186264e90R57-R69) * Added logic to ensure all cursors are closed before closing the connection and made `close()` idempotent to handle multiple calls safely. (`mssql_python/connection.py`) ### Cursor Handling Improvements: * Updated the `cursor()` method to raise an exception if called on a closed connection and track active cursors using `WeakSet`. (`mssql_python/connection.py`) ### Test Case Updates: * Added new tests to validate cursor cleanup behavior, weak reference handling, cleanup order, and connection idempotency. (`tests/test_003_connection.py`) * Updated stored procedure-related tests to use the `dbo` schema prefix for better compatibility and clarity. (`tests/test_004_cursor.py`) [[1]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L873-R879) [[2]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L895-R895) [[3]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L907-R907) [[4]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L918-R918) * Modified table creation and cleanup logic in multiple test cases to use temporary tables (`#pytest_*`) for isolation and reduced side effects. (`tests/test_004_cursor.py`, `tests/test_005_exceptions.py`) [[1]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L1153-R1153) [[2]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L1164-R1170) [[3]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L1192-R1231) [[4]](diffhunk://#diff-82594712308ff34afa8b067af67db231e9a1372ef474da3db121e14e4d418f69L1240-R1240) [[5]](diffhunk://#diff-282f76cf72519247beed3fe56aa378a339af5ecbdef32fb468a3470969d15b69L88-R128) <!-- ### 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 6961abb commit a9632fb

File tree

5 files changed

+281
-41
lines changed

5 files changed

+281
-41
lines changed

mssql_python/connection.py

Lines changed: 67 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,14 +4,20 @@
44
This module defines the Connection class, which is used to manage a connection to a database.
55
The class provides methods to establish a connection, create cursors, commit transactions,
66
roll back transactions, and close the connection.
7+
Resource Management:
8+
- All cursors created from this connection are tracked internally.
9+
- When close() is called on the connection, all open cursors are automatically closed.
10+
- Do not use any cursor after the connection is closed; doing so will raise an exception.
11+
- Cursors are also cleaned up automatically when no longer referenced, to prevent memory leaks.
712
"""
8-
import ctypes
13+
import weakref
914
from mssql_python.cursor import Cursor
1015
from mssql_python.logging_config import get_logger, ENABLE_LOGGING
1116
from mssql_python.constants import ConstantsDDBC as ddbc_sql_const
1217
from mssql_python.helpers import add_driver_to_connection_str, check_error
1318
from mssql_python import ddbc_bindings
1419
from mssql_python.pooling import PoolingManager
20+
from mssql_python.exceptions import DatabaseError, InterfaceError
1521

1622
logger = get_logger()
1723

@@ -58,6 +64,15 @@ def __init__(self, connection_str: str = "", autocommit: bool = False, attrs_bef
5864
connection_str, **kwargs
5965
)
6066
self._attrs_before = attrs_before or {}
67+
self._closed = False
68+
69+
# Using WeakSet which automatically removes cursors when they are no longer in use
70+
# It is a set that holds weak references to its elements.
71+
# When an object is only weakly referenced, it can be garbage collected even if it's still in the set.
72+
# It prevents memory leaks by ensuring that cursors are cleaned up when no longer in use without requiring explicit deletion.
73+
# TODO: Think and implement scenarios for multi-threaded access to cursors
74+
self._cursors = weakref.WeakSet()
75+
6176
# Auto-enable pooling if user never called
6277
if not PoolingManager.is_initialized():
6378
PoolingManager.enable()
@@ -152,7 +167,17 @@ def cursor(self) -> Cursor:
152167
DatabaseError: If there is an error while creating the cursor.
153168
InterfaceError: If there is an error related to the database interface.
154169
"""
155-
return Cursor(self)
170+
"""Return a new Cursor object using the connection."""
171+
if self._closed:
172+
# raise InterfaceError
173+
raise InterfaceError(
174+
driver_error="Cannot create cursor on closed connection",
175+
ddbc_error="Cannot create cursor on closed connection",
176+
)
177+
178+
cursor = Cursor(self)
179+
self._cursors.add(cursor) # Track the cursor
180+
return cursor
156181

157182
def commit(self) -> None:
158183
"""
@@ -201,6 +226,45 @@ def close(self) -> None:
201226
DatabaseError: If there is an error while closing the connection.
202227
"""
203228
# Close the connection
204-
self._conn.close()
229+
if self._closed:
230+
return
231+
232+
# Close all cursors first, but don't let one failure stop the others
233+
if hasattr(self, '_cursors'):
234+
# Convert to list to avoid modification during iteration
235+
cursors_to_close = list(self._cursors)
236+
close_errors = []
237+
238+
for cursor in cursors_to_close:
239+
try:
240+
if not cursor.closed:
241+
cursor.close()
242+
except Exception as e:
243+
# Collect errors but continue closing other cursors
244+
close_errors.append(f"Error closing cursor: {e}")
245+
if ENABLE_LOGGING:
246+
logger.warning(f"Error closing cursor: {e}")
247+
248+
# If there were errors closing cursors, log them but continue
249+
if close_errors and ENABLE_LOGGING:
250+
logger.warning(f"Encountered {len(close_errors)} errors while closing cursors")
251+
252+
# Clear the cursor set explicitly to release any internal references
253+
self._cursors.clear()
254+
255+
# Close the connection even if cursor cleanup had issues
256+
try:
257+
if self._conn:
258+
self._conn.close()
259+
self._conn = None
260+
except Exception as e:
261+
if ENABLE_LOGGING:
262+
logger.error(f"Error closing database connection: {e}")
263+
# Re-raise the connection close error as it's more critical
264+
raise
265+
finally:
266+
# Always mark as closed, even if there were errors
267+
self._closed = True
268+
205269
if ENABLE_LOGGING:
206270
logger.info("Connection closed successfully.")

mssql_python/cursor.py

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,11 @@
22
Copyright (c) Microsoft Corporation.
33
Licensed under the MIT license.
44
This module contains the Cursor class, which represents a database cursor.
5+
Resource Management:
6+
- Cursors are tracked by their parent connection.
7+
- Closing the connection will automatically close all open cursors.
8+
- Do not use a cursor after it is closed, or after its parent connection is closed.
9+
- Use close() to release resources held by the cursor as soon as it is no longer needed.
510
"""
611
import ctypes
712
import decimal

tests/test_003_connection.py

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

13+
from mssql_python.exceptions import InterfaceError
1314
import pytest
1415
import time
1516
from mssql_python import Connection, connect, pooling
1617

1718
def drop_table_if_exists(cursor, table_name):
1819
"""Drop the table if it exists"""
1920
try:
20-
cursor.execute(f"IF OBJECT_ID('{table_name}', 'U') IS NOT NULL DROP TABLE {table_name}")
21+
cursor.execute(f"DROP TABLE IF EXISTS {table_name}")
2122
except Exception as e:
2223
pytest.fail(f"Failed to drop table {table_name}: {e}")
2324

@@ -223,4 +224,172 @@ def test_connection_pooling_basic(conn_str):
223224

224225
conn1.close()
225226
conn2.close()
226-
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)