FIX: Cursor resource management, error handling & logging

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
+        return result_rows

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)
 
 

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")

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
+    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