-
Notifications
You must be signed in to change notification settings - Fork 30
FIX: Fix for isolation level for in-built connection pool #343
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this 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 a critical bug in connection pooling where the transaction isolation level was not being reset when connections were returned to and reused from the pool. The fix ensures that READ COMMITTED (the SQL Server default) is explicitly set during connection reset, preventing session state leakage between pooled connection usages.
Key changes:
- Added explicit transaction isolation level reset to
READ COMMITTEDin theConnection::reset()method afterSQL_ATTR_RESET_CONNECTIONis called - Added comprehensive test to verify that isolation level is properly reset when connections are reused from the pool
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mssql_python/pybind/connection/connection.cpp | Adds explicit reset of transaction isolation level to SQL_TXN_READ_COMMITTED in the Connection::reset() method to prevent isolation level leakage between pooled connection usages |
| tests/test_009_pooling.py | Imports mssql_python module and adds test_connection_pooling_isolation_level_reset test to verify isolation level is reset to default when connections are reused from the pool |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/connection/connection.cppLines 315-326 315 LOG("Resetting transaction isolation level to READ COMMITTED");
316 ret = SQLSetConnectAttr_ptr(_dbcHandle->get(), SQL_ATTR_TXN_ISOLATION,
317 (SQLPOINTER)SQL_TXN_READ_COMMITTED, SQL_IS_INTEGER);
318 if (!SQL_SUCCEEDED(ret)) {
! 319 LOG("Failed to reset transaction isolation level (ret=%d). Marking as dead.", ret);
! 320 disconnect();
! 321 return false;
! 322 }
323
324 updateLastUsed();
325 return true;
326 }📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.row.py: 66.2%
mssql_python.pybind.ddbc_bindings.cpp: 67.1%
mssql_python.helpers.py: 67.5%
mssql_python.pybind.connection.connection.cpp: 73.6%
mssql_python.pybind.ddbc_bindings.h: 76.9%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 79.6%
mssql_python.connection.py: 82.5%
mssql_python.cursor.py: 83.8%🔗 Quick Links
|
saurabh500
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need verification for Txn Level altered with ALTER DATABASE
saurabh500
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Provided some feedback. For its current scope the PR looks good.
Summary
This pull request addresses a critical issue in connection pooling for the
mssql_pythonlibrary by ensuring that the transaction isolation level is properly reset when a pooled connection is reused. Previously, the isolation level could leak between usages, potentially causing unexpected behavior. The changes include an explicit reset of the isolation level in the connection reset logic and a new test to verify this behavior.Bug fix for connection pooling session state:
READ COMMITTEDin theConnection::reset()method to prevent isolation level settings from leaking between pooled connection usages. This addresses the limitation ofSQL_ATTR_RESET_CONNECTION, which does not reset the isolation level. (mssql_python/pybind/connection/connection.cpp)Testing improvements:
test_connection_pooling_isolation_level_resetintests/test_009_pooling.pyto verify that the isolation level is reset to the default when a connection is reused from the pool, preventing session state leakage.mssql_pythonmodule intests/test_009_pooling.pyto support new test functionality.