FIX: prevent GIL/mutex deadlock when logging under concurrent multithreaded use

[Gaurav Sharma (bewithgaurav)]

Work Item / Issue Reference

ADO Work Item: Fixed AB#46334
GitHub Issue: #671

---

Summary

This pull request addresses a critical deadlock issue (#671) that could occur when DEBUG logging is enabled and multiple threads interact concurrently with the connection pool and logging system. The main changes ensure that any logging (which acquires Python's GIL) is never performed while holding native mutexes, thus preventing lock-order inversions and deadlocks. It also adds regression tests to guarantee that this deadlock does not return.

Deadlock Prevention and Logging Safety:

• Refactored all code paths in connection.cpp and connection_pool.cpp so that logging calls (LOG, LOG_ERROR) are made only after releasing any native mutexes, preventing GIL/native-mutex lock-order inversions that caused deadlocks under concurrent DEBUG logging. Comments were added to explain the rationale and reference issue Deadlock under concurrent multithreaded use when DEBUG logging is enabled via setup_logging() #671. [1] [2] [3] [4] [5]
• In logger_bridge.cpp, removed the use of a native mutex in the logging bridge, relying solely on the GIL for thread safety, which avoids deadlocks when logging from multiple threads.

Connection Pooling and Handle Management:

• Updated connection pool logic to construct Connection objects and perform ODBC handle allocation outside of pool mutexes, ensuring that any logging or GIL acquisition happens only after releasing native locks. [1] [2] [3] [4]
• Improved child handle compaction and logging in Connection::disconnect() and allocStatementHandle() by collecting compacted handle statistics while holding the mutex, but deferring logging until after the mutex is released. [1] [2]

Testing and Regression Coverage:

• Added a new regression test file test_025_logging_concurrency_deadlock.py that launches multi-threaded workloads under DEBUG logging in a subprocess, verifying that no deadlocks occur. The test is robust, runs out-of-process, and will fail if the deadlock regresses.

These changes collectively ensure that the driver is robust against concurrency-related deadlocks when logging is enabled, and that future regressions will be caught by automated tests.

[github-actions]

📊 Code Coverage Report

🔥 Diff Coverage 92%

🎯 Overall Coverage 82%

📈 Total Lines Covered: 7770 out of 9451 📁 Project: mssql-python

---

Diff Coverage

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

• mssql_python/pybind/connection/connection.cpp (90.9%): Missing lines 142,159-160
• mssql_python/pybind/connection/connection_pool.cpp (100%)

Summary

• Total: 39 lines
• Missing: 3 lines
• Coverage: 92%

mssql_python/pybind/connection/connection.cpp

Lines 138-146

  138                     // SAFETY ASSERTION: Only STMT handles should be in this vector
  139                     // This is guaranteed by allocStatementHandle() which only creates STMT handles
  140                     // If this assertion fails, it indicates a serious bug in handle tracking
  141                     if (handle->type() != SQL_HANDLE_STMT) {
! 142                         ++badHandleCount;
  143                         continue;  // Skip marking to prevent leak
  144                     }
  145                     handle->markImplicitlyFreed();
  146                 }

---

Lines 155-164

  155         if (hasGil) {
  156             LOG("Compacted child handles: %zu -> %zu (removed %zu expired)",
  157                 originalSize, afterCompactSize, originalSize - afterCompactSize);
  158             LOG("Marking %zu child statement handles as implicitly freed", afterCompactSize);
! 159             if (badHandleCount > 0) {
! 160                 LOG_ERROR("CRITICAL: %zu non-STMT handle(s) found in _childStatementHandles. "
  161                           "This will cause a handle leak!", badHandleCount);
  162             }
  163         }

---

---

📋 Files Needing Attention

📉 Files with overall lowest coverage (click to expand)

mssql_python.pybind.logger_bridge.cpp: 58.9%
mssql_python.pybind.ddbc_bindings.h: 61.5%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 75.5%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.6%
mssql_python.pybind.connection.connection.cpp: 84.5%
mssql_python.logging.py: 85.5%

---

🔗 Quick Links

⚙️ Build Summary	📋 Coverage Details
View Azure DevOps Build	Browse Full Coverage Report

[Copilot]

Pull request overview

Fixes a native/Python lock-order inversion that could deadlock multithreaded workloads when DEBUG logging is enabled, by ensuring native logging never holds critical C++ mutexes (or static-init guards) across GIL acquisition and by restructuring pooled-connection creation to avoid constructing/logging under pool locks.

Changes:

• Removed the logger bridge’s native mutex from the hot logging path and relied on the GIL for Python logging serialization.
• Refactored connection, pooling, and child-handle tracking paths to avoid calling LOG() while holding native mutexes; released the GIL around env-handle static initialization.
• Added an out-of-process regression test that asserts the concurrent DEBUG-logged workload completes (no deadlock).

Reviewed changes

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

File	Description
tests/test_025_logging_concurrency_deadlock.py	Adds a subprocess-based regression test for the DEBUG-logging + concurrency deadlock.
mssql_python/pybind/logger_bridge.cpp	Removes native mutex acquisition before GIL acquisition in the C++→Python logging bridge.
mssql_python/pybind/connection/connection.cpp	Avoids logging while holding connection child-handle mutex; releases GIL around env-handle static init.
mssql_python/pybind/connection/connection_pool.cpp	Avoids constructing/logging under pool/manager mutexes by reserving slots then constructing/logging outside locks.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.