Skip to content

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

Draft
bewithgaurav wants to merge 5 commits into
mainfrom
bewithgaurav/fix-671-logging-deadlock
Draft

FIX: prevent GIL/mutex deadlock when logging under concurrent multithreaded use#678
bewithgaurav wants to merge 5 commits into
mainfrom
bewithgaurav/fix-671-logging-deadlock

Conversation

@bewithgaurav

Copy link
Copy Markdown
Collaborator

Work Item / Issue Reference

GitHub Issue: #671


Summary

native LOG() from C++ routes records through Python's logging, so it acquires
the GIL. several sites did that while holding a native lock (the connection-pool
mutexes, the per-connection child-handles mutex, the logger's own mutex) or the
getEnvHandle static-init guard. with DEBUG logging on and 2+ threads connecting
or executing, that inverts lock order against a thread that holds the GIL and is
waiting on the same native lock, so the process deadlocks at 0% cpu. logging
off, or single-threaded, never deadlocks.

fix makes native logging never hold a native lock across a GIL acquisition:
build the log values under the lock and log after releasing it, construct the
pooled Connection outside the pool mutex, release the GIL around the env-handle
init, and drop the mutex the logger took before the GIL.

reproduced the report locally and confirmed the fix: no deadlock at
1/2/4/8/12/16 threads with logging on (was a permanent hang at 4). logging
output is unchanged (native + python records still written), and the
logging / pooling / connection / lifecycle / multithreaded-stress suites pass.

bewithgaurav and others added 2 commits July 13, 2026 19:44
…readed use

native LOG() routes records through Python's logging, so it acquires the GIL
from C++. several sites did that while holding a native mutex (the
connection-pool mutexes, the per-connection child-handles mutex, the logger's
own mutex) or the getEnvHandle static-init guard. under concurrent use with
DEBUG logging on, that inverts lock order against a thread that holds the GIL
and is waiting on the same native lock, so the process deadlocks at 0% cpu.

this makes native logging never hold a native lock across a GIL acquisition:
build the log values under the lock, release the lock, then log; construct the
Connection and close pools outside the pool mutexes; release the GIL around the
env-handle init.

verified no deadlock at 1/2/4/8/12/16 threads with logging on (was 100%
deadlock at 4 threads). logging output and the pooling/logging/stress test
suites are unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
 paths

the close-pooling path has the same lock/GIL shape but a different trigger
(close running concurrently with active connects) and it interacts with a
separate, pre-existing pool-accounting concern in the return path. reverting
that hunk here keeps this PR limited to the connect / normal-use deadlock the
issue reports. closePools() returns to its original behavior; the five fixed
sites (logger bridge, pool acquire, acquireConnection, getEnvHandle, child
handle logging) are unchanged.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added the pr-size: small Minimal code update label Jul 14, 2026
@github-actions

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

90%


🎯 Overall Coverage

80%


📈 Total Lines Covered: 6759 out of 8361
📁 Project: mssql-python


Diff Coverage

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

  • mssql_python/pybind/connection/connection.cpp (88.5%): Missing lines 135,150-151
  • mssql_python/pybind/connection/connection_pool.cpp (100%)

Summary

  • Total: 32 lines
  • Missing: 3 lines
  • Coverage: 90%

mssql_python/pybind/connection/connection.cpp

Lines 131-139

  131                     // SAFETY ASSERTION: Only STMT handles should be in this vector
  132                     // This is guaranteed by allocStatementHandle() which only creates STMT handles
  133                     // If this assertion fails, it indicates a serious bug in handle tracking
  134                     if (handle->type() != SQL_HANDLE_STMT) {
! 135                         ++badHandleCount;
  136                         continue;  // Skip marking to prevent leak
  137                     }
  138                     handle->markImplicitlyFreed();
  139                 }

Lines 146-155

  146         // the GIL and must not run while a native mutex is held.
  147         LOG("Compacted child handles: %zu -> %zu (removed %zu expired)",
  148             originalSize, afterCompactSize, originalSize - afterCompactSize);
  149         LOG("Marking %zu child statement handles as implicitly freed", afterCompactSize);
! 150         if (badHandleCount > 0) {
! 151             LOG_ERROR("CRITICAL: %zu non-STMT handle(s) found in _childStatementHandles. "
  152                       "This will cause a handle leak!", badHandleCount);
  153         }
  154 
  155         SQLRETURN ret;


📋 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: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.2%
mssql_python.pybind.connection.connection.cpp: 76.5%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 80.4%
mssql_python.connection.py: 83.6%

🔗 Quick Links

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

bewithgaurav and others added 2 commits July 15, 2026 12:32
adds a functional test (default suite) and a stress test (@pytest.mark.stress)
that run DEBUG logging + concurrent connect/execute and assert the process does
not deadlock. the workload runs in a child process with a wall-clock timeout so
a real GIL/native-mutex deadlock (which freezes the interpreter and can't be
interrupted in-process) is turned into a clean failure, and each run gets a
fresh logging singleton so it can't leak into the rest of the suite.

verified the functional test fails (times out) against the pre-fix build and
passes in ~5s against the fixed build.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions github-actions Bot added pr-size: medium Moderate update size and removed pr-size: small Minimal code update labels Jul 15, 2026
…ault run

removes the standalone tests/_issue671_deadlock_workload.py script (an odd
non-test file in tests/) and moves the workload into a __main__ block in
test_025, which the tests invoke as the child process. one self-contained file,
matching the layout of the other test modules.

also drops the default (non-stress) test to 2 threads to match the repo's
lightest concurrency baseline (test_020's (2, 50)); 2 threads is issue #671's
stated minimum and still deadlocks the pre-fix build 3/3 runs, so the guard
stays reliable without loading PR validation. the heavy 16-thread variant stays
under @pytest.mark.stress. log file now uses pytest's tmp_path (auto-cleaned).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: medium Moderate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant