FIX: Closed Attribute in cursor __del__ (#183)

### Work Item / Issue Reference <!-- IMPORTANT: Please follow the PR template guidelines below. For mssql-python maintainers: Insert your ADO Work Item ID below (e.g. AB#37452) For external contributors: Insert Github Issue number below (e.g. #149) Only one reference is required - either GitHub issue OR ADO Work Item. --> <!-- mssql-python maintainers: ADO Work Item --> > AB#<WORK_ITEM_ID> <!-- External contributors: GitHub Issue --> > GitHub Issue: #182 ------------------------------------------------------------------- ### Summary <!-- Insert your summary of changes below. Minimum 10 characters required. --> This pull request makes improvements to the cursor resource management and error handling in the `mssql_python/cursor.py` module. The main changes include making the cursor close operation idempotent, refining exception types, and improving logging during cleanup. These updates help ensure more robust and predictable behavior when working with cursors. Error handling improvements: * Changed the exception raised when attempting to close an already closed cursor from a generic `Exception` to a more specific `ProgrammingError`, providing clearer feedback to users. * Updated the import statement to use `ProgrammingError` instead of `InterfaceError`, aligning with the new error handling approach. Resource cleanup and logging: * Improved the destructor (`__del__`) to check for the correct `closed` attribute and made sure no exception is raised if the cursor is already closed, enhancing reliability during garbage collection. * Changed the logging level in the destructor from 'error' to 'debug' for exceptions during cleanup, reducing unnecessary noise in error logs. <!-- ### 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) ### Contribution Guidelines External contributors: - Create a GitHub issue first: https://github.com/microsoft/mssql-python/issues/new - Link the GitHub issue in the "GitHub Issue" section above - Follow the PR title format and provide a meaningful summary mssql-python maintainers: - Create an ADO Work Item following internal processes - Link the ADO Work Item in the "ADO Work Item" section above - Follow the PR title format and provide a meaningful summary -->

Author: bewithgaurav

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