diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index e2c811c9..5c54f97d 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -471,17 +471,16 @@ def _reset_cursor(self) -> None: def close(self) -> None: """ - Close the cursor now (rather than whenever __del__ is called). + Close the connection now (rather than whenever .__del__() is called). + Idempotent: subsequent calls have no effect and will be no-ops. The cursor will be unusable from this point forward; an InterfaceError - will be raised if any operation is attempted with the cursor. - - Note: - Unlike the current behavior, this method can be called multiple times safely. - Subsequent calls to close() on an already closed cursor will have no effect. + will be raised if any operation (other than close) is attempted with the cursor. + This is a deviation from pyodbc, which raises an exception if the cursor is already closed. """ if self.closed: - return + # Do nothing - not calling _check_closed() here since we want this to be idempotent + return # Clear messages per DBAPI self.messages = [] @@ -498,12 +497,12 @@ def _check_closed(self): Check if the cursor is closed and raise an exception if it is. Raises: - InterfaceError: If the cursor is closed. + ProgrammingError: If the cursor is closed. """ if self.closed: - raise InterfaceError( - driver_error="Operation cannot be performed: the cursor is closed.", - ddbc_error="Operation cannot be performed: the cursor is closed." + raise ProgrammingError( + driver_error="Operation cannot be performed: The cursor is closed.", + ddbc_error="" ) def _create_parameter_types_list(self, parameter, param_info, parameters_list, i): @@ -1185,13 +1184,19 @@ def __del__(self): Destructor to ensure the cursor is closed when it is no longer needed. This is a safety net to ensure resources are cleaned up even if close() was not called explicitly. + If the cursor is already closed, it will not raise an exception during cleanup. """ - if "_closed" not in self.__dict__ or not self._closed: + if "closed" not in self.__dict__ or not self.closed: try: self.close() except Exception as e: # Don't raise an exception in __del__, just log it - log('error', "Error during cursor cleanup in __del__: %s", e) + # If interpreter is shutting down, we might not have logging set up + import sys + if sys and sys._is_finalizing(): + # Suppress logging during interpreter shutdown + return + log('debug', "Exception during cursor cleanup in __del__: %s", e) def scroll(self, value: int, mode: str = 'relative') -> None: """ @@ -1430,4 +1435,4 @@ def tables(self, table=None, catalog=None, schema=None, tableType=None): row = Row(row_data, self.description, column_map) result_rows.append(row) - return result_rows \ No newline at end of file + return result_rows diff --git a/mssql_python/exceptions.py b/mssql_python/exceptions.py index 308a8569..93530347 100644 --- a/mssql_python/exceptions.py +++ b/mssql_python/exceptions.py @@ -17,9 +17,14 @@ class Exception(Exception): def __init__(self, driver_error, ddbc_error) -> None: self.driver_error = driver_error self.ddbc_error = truncate_error_message(ddbc_error) - self.message = ( - f"Driver Error: {self.driver_error}; DDBC Error: {self.ddbc_error}" - ) + if self.ddbc_error: + # Both driver and DDBC errors are present + self.message = ( + f"Driver Error: {self.driver_error}; DDBC Error: {self.ddbc_error}" + ) + else: + # Errors raised by the driver itself should not have a DDBC error message + self.message = f"Driver Error: {self.driver_error}" super().__init__(self.message) diff --git a/tests/test_003_connection.py b/tests/test_003_connection.py index 28ab9286..576c49bf 100644 --- a/tests/test_003_connection.py +++ b/tests/test_003_connection.py @@ -1528,7 +1528,7 @@ def test_connection_exception_catching_with_connection_attributes(db_connection) cursor.close() cursor.execute("SELECT 1") # Should raise InterfaceError on closed cursor pytest.fail("Should have raised an exception") - except db_connection.InterfaceError as e: + except db_connection.ProgrammingError as e: assert "closed" in str(e).lower(), "Error message should mention closed cursor" except Exception as e: pytest.fail(f"Should have caught InterfaceError, but got {type(e).__name__}: {e}") @@ -1567,12 +1567,12 @@ def test_connection_exception_multi_connection_scenario(conn_str): try: cursor1.execute("SELECT 1") pytest.fail("Should have raised an exception") - except conn1.InterfaceError as e: - # Using conn1.InterfaceError even though conn1 is closed + except conn1.ProgrammingError as e: + # Using conn1.ProgrammingError even though conn1 is closed # The exception class attribute should still be accessible assert "closed" in str(e).lower(), "Should mention closed cursor" except Exception as e: - pytest.fail(f"Expected InterfaceError from conn1 attributes, got {type(e).__name__}: {e}") + pytest.fail(f"Expected ProgrammingError from conn1 attributes, got {type(e).__name__}: {e}") # Second connection should still work cursor2.execute("SELECT 1") diff --git a/tests/test_005_connection_cursor_lifecycle.py b/tests/test_005_connection_cursor_lifecycle.py index 5fa3d56c..d87c3f21 100644 --- a/tests/test_005_connection_cursor_lifecycle.py +++ b/tests/test_005_connection_cursor_lifecycle.py @@ -13,8 +13,15 @@ - test_connection_close_idempotent: Tests that calling close() multiple times is safe. - test_cursor_after_connection_close: Tests that creating a cursor after closing the connection raises an error. - test_multiple_cursor_operations_cleanup: Tests cleanup with multiple cursor operations. +- test_cursor_close_raises_on_double_close: Tests that closing a cursor twice raises a ProgrammingError. +- test_cursor_del_no_logging_during_shutdown: Tests that cursor __del__ doesn't log errors during interpreter shutdown. +- test_cursor_del_on_closed_cursor_no_errors: Tests that __del__ on already closed cursor doesn't produce error logs. +- test_cursor_del_unclosed_cursor_cleanup: Tests that __del__ properly cleans up unclosed cursors without errors. +- test_cursor_operations_after_close_raise_errors: Tests that all cursor operations raise appropriate errors after close. +- test_mixed_cursor_cleanup_scenarios: Tests various mixed cleanup scenarios in one script. """ +import os import pytest import subprocess import sys @@ -263,4 +270,208 @@ def test_multiple_cursor_operations_cleanup(conn_str): # All cursors should be closed assert cursor_insert.closed is True assert cursor_select1.closed is True - assert cursor_select2.closed is True \ No newline at end of file + assert cursor_select2.closed is True + +def test_cursor_close_raises_on_double_close(conn_str): + """Test that closing a cursor twice raises ProgrammingError""" + conn = connect(conn_str) + cursor = conn.cursor() + cursor.execute("SELECT 1") + cursor.fetchall() + + # First close should succeed + cursor.close() + assert cursor.closed is True + + # Second close should be a no-op and silent - not raise an error + cursor.close() + assert cursor.closed is True + + +def test_cursor_del_no_logging_during_shutdown(conn_str, tmp_path): + """Test that cursor __del__ doesn't log errors during interpreter shutdown""" + code = f""" +from mssql_python import connect + +# Create connection and cursor +conn = connect(\"\"\"{conn_str}\"\"\") +cursor = conn.cursor() +cursor.execute("SELECT 1") +cursor.fetchall() + +# Don't close cursor - let __del__ handle it during shutdown +# This should not produce any log output during interpreter shutdown +print("Test completed successfully") +""" + + result = subprocess.run( + [sys.executable, "-c", code], + capture_output=True, + text=True + ) + + # Should exit cleanly + assert result.returncode == 0, f"Script failed: {result.stderr}" + # Should not have any debug/error logs about cursor cleanup + assert "Exception during cursor cleanup" not in result.stderr + assert "Exception during cursor cleanup" not in result.stdout + # Should have our success message + assert "Test completed successfully" in result.stdout + + +def test_cursor_del_on_closed_cursor_no_errors(conn_str, caplog): + """Test that __del__ on already closed cursor doesn't produce error logs""" + import logging + caplog.set_level(logging.DEBUG) + + conn = connect(conn_str) + cursor = conn.cursor() + cursor.execute("SELECT 1") + cursor.fetchall() + + # Close cursor explicitly + cursor.close() + + # Clear any existing logs + caplog.clear() + + # Delete the cursor - should not produce any logs + del cursor + import gc + gc.collect() + + # Check that no error logs were produced + for record in caplog.records: + assert "Exception during cursor cleanup" not in record.message + assert "Operation cannot be performed: The cursor is closed." not in record.message + + conn.close() + + +def test_cursor_del_unclosed_cursor_cleanup(conn_str): + """Test that __del__ properly cleans up unclosed cursors without errors""" + code = f""" +from mssql_python import connect + +# Create connection and cursor +conn = connect(\"\"\"{conn_str}\"\"\") +cursor = conn.cursor() +cursor.execute("SELECT 1") +cursor.fetchall() + +# Store cursor state before deletion +cursor_closed_before = cursor.closed + +# Delete cursor without closing - __del__ should handle cleanup +del cursor +import gc +gc.collect() + +# Verify cursor was not closed before deletion +assert cursor_closed_before is False, "Cursor should not be closed before deletion" + +# Close connection +conn.close() +print("Cleanup successful") +""" + + result = subprocess.run( + [sys.executable, "-c", code], + capture_output=True, + text=True + ) + assert result.returncode == 0, f"Expected successful cleanup, but got: {result.stderr}" + assert "Cleanup successful" in result.stdout + # Should not have any error messages + assert "Exception" not in result.stderr + + +def test_cursor_operations_after_close_raise_errors(conn_str): + """Test that all cursor operations raise appropriate errors after close""" + conn = connect(conn_str) + cursor = conn.cursor() + cursor.execute("SELECT 1") + cursor.fetchall() + + # Close the cursor + cursor.close() + + # All operations should raise exceptions + with pytest.raises(Exception) as excinfo: + cursor.execute("SELECT 2") + assert "Operation cannot be performed: The cursor is closed." in str(excinfo.value) + + with pytest.raises(Exception) as excinfo: + cursor.fetchone() + assert "Operation cannot be performed: The cursor is closed." in str(excinfo.value) + + with pytest.raises(Exception) as excinfo: + cursor.fetchmany(5) + assert "Operation cannot be performed: The cursor is closed." in str(excinfo.value) + + with pytest.raises(Exception) as excinfo: + cursor.fetchall() + assert "Operation cannot be performed: The cursor is closed." in str(excinfo.value) + + conn.close() + + +def test_mixed_cursor_cleanup_scenarios(conn_str, tmp_path): + """Test various mixed cleanup scenarios in one script""" + code = f""" +from mssql_python import connect +from mssql_python.exceptions import ProgrammingError + +# Test 1: Normal cursor close +conn1 = connect(\"\"\"{conn_str}\"\"\") +cursor1 = conn1.cursor() +cursor1.execute("SELECT 1") +cursor1.fetchall() +cursor1.close() + +# Test 2: Double close does not raise error +cursor1.close() +print("PASS: Double close does not raise error") + +# Test 3: Cursor cleanup via __del__ +cursor2 = conn1.cursor() +cursor2.execute("SELECT 2") +cursor2.fetchall() +# Don't close cursor2, let __del__ handle it + +# Test 4: Connection close cleans up cursors +conn2 = connect(\"\"\"{conn_str}\"\"\") +cursor3 = conn2.cursor() +cursor4 = conn2.cursor() +cursor3.execute("SELECT 3") +cursor3.fetchall() +cursor4.execute("SELECT 4") +cursor4.fetchall() +conn2.close() # Should close both cursors + +# Verify cursors are closed +assert cursor3.closed is True +assert cursor4.closed is True +print("PASS: Connection close cleaned up cursors") + +# Clean up +conn1.close() +print("All tests passed") +""" + + result = subprocess.run( + [sys.executable, "-c", code], + capture_output=True, + text=True + ) + + if result.returncode != 0: + print(f"STDOUT: {result.stdout}") + print(f"STDERR: {result.stderr}") + + assert result.returncode == 0, f"Script failed: {result.stderr}" + assert "PASS: Double close does not raise error" in result.stdout + assert "PASS: Connection close cleaned up cursors" in result.stdout + assert "All tests passed" in result.stdout + # Should not have error logs + assert "Exception during cursor cleanup" not in result.stderr