Skip to content

Conversation

bewithgaurav
Copy link
Collaborator

Work Item / Issue Reference

GitHub Issue: Closes #267


Summary

This pull request improves the reliability and testability of the connection pooling logic in mssql_python/pooling.py by ensuring that pool shutdown and cleanup operations are properly tracked and only performed once. It also adds a method to reset the pooling state for testing purposes.

Connection pool shutdown safety:

  • Added a _pools_closed flag to the PoolingManager class to track whether pools have already been closed, preventing multiple shutdowns and resource leaks.
  • Updated the disable method and the shutdown_pooling function to check _pools_closed before attempting to close pools, ensuring cleanup only happens once. [1] [2]

Testing support:

  • Added a _reset_for_testing class method to PoolingManager to reset internal state, making it easier to write reliable tests for pooling behavior.

@Copilot Copilot AI review requested due to automatic review settings October 1, 2025 05:23
@github-actions github-actions bot added the pr-size: large Substantial code update label Oct 1, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request fixes connection pooling resource cleanup by preventing multiple shutdown operations and resource leaks, while also improving test isolation for pooling functionality.

  • Adds tracking of pool closure state to prevent double-cleanup operations
  • Introduces thread-safe cleanup logic that only runs once when pools are enabled
  • Adds a testing utility method to reset pooling state for better test isolation

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
mssql_python/pooling.py Implements pool closure tracking and thread-safe cleanup logic to prevent resource leaks
tests/test_009_pooling.py Comprehensive new test suite for pooling functionality including bug fix verification tests
tests/test_003_connection.py Removes pooling tests that were moved to the dedicated pooling test file

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

github-actions bot commented Oct 1, 2025

📊 Code Coverage Report

🔥 Diff Coverage

71%


🎯 Overall Coverage

74%


📈 Total Lines Covered: 4181 out of 5612
📁 Project: mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/pooling.py (71.4%): Missing lines 58-61

Summary

  • Total: 14 lines
  • Missing: 4 lines
  • Coverage: 71%

mssql_python/pooling.py

Lines 54-62

  54             cls._pools_closed = False
  55     
  56 @atexit.register
  57 def shutdown_pooling():
! 58     with PoolingManager._lock:
! 59         if PoolingManager._enabled and not PoolingManager._pools_closed:
! 60             ddbc_bindings.close_pooling()
! 61             PoolingManager._pools_closed = True


📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)
mssql_python.pybind.connection.connection.cpp: 67.6%
mssql_python.ddbc_bindings.py: 68.5%
mssql_python.pybind.ddbc_bindings.cpp: 69.3%
mssql_python.pybind.connection.connection_pool.cpp: 78.9%
mssql_python.cursor.py: 79.7%
mssql_python.connection.py: 81.7%
mssql_python.helpers.py: 84.7%
mssql_python.auth.py: 85.3%
mssql_python.type.py: 86.8%
mssql_python.pooling.py: 87.5%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@bewithgaurav bewithgaurav merged commit 200c35b into main Oct 8, 2025
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-size: large Substantial code update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connection pooling disable function hangs indefinitely
3 participants