Repository navigation
FIX: Bound pooled connection checkout on unresponsive sockets - #859
Conversation
Apply a temporary native connection timeout during pooled reset and autocommit configuration, then restore it on success. Replace a failed candidate instead of probing all idle sockets sequentially. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
PR Performance Report✅ No regression detectedNo consistent slowdowns detected across all 2 environments. 0 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS Coverage: 2 of 2 environments completed. Advisory result; does not block merging. Performance diagnosticsPhase times are inclusive diagnostics and must not be added together. They identify where measured time changed, not why it changed. Unix / SQL Server 2022Connection opening: ddbc::ConnectionPoolManager::acquireConnection +0.336 ms; ddbc::ConnectionHandle::ConnectionHandle +0.336 ms; ddbc::ConnectionPool::acquire +0.332 ms. Call changes: ddbc::Connection::setAutocommit (added, removed, or intermittent); ddbc::ConnectionHandle::setAutocommit (added, removed, or intermittent). Unix / SQL Server 2025Connection opening: no measured phase delta. Call changes: ddbc::Connection::setAutocommit (added, removed, or intermittent); ddbc::ConnectionHandle::setAutocommit (added, removed, or intermittent). All database tasks and timingsUnix / SQL Server 2022
Unix / SQL Server 2025
Build and measurement detailsPR head:
A consistent change requires more than 20% median paired movement, at least 1 ms between the median runtimes, and at least 80% of pairs exceeding the relative threshold in the same direction. A slowdown without enough pair agreement is reported as inconsistent. The displayed change is the median of paired before-and-after ratios. It is not recalculated from the two displayed median runtimes. Both revisions use profiling-enabled builds on the same agent and database, with alternating order and discarded warmups. Results are diagnostic and do not represent production-wheel latency. Raw samples and logs are attached to the ADO run as |
There was a problem hiding this comment.
🔵 Needs a closer look
The reported native crash remains unexplained, and Windows/macOS timeout and cleanup behavior still needs validation.
1 open issue
What changed in this PR
Addresses #854 by limiting pooled-connection validation on unresponsive sockets without imposing an end-to-end connect() deadline.
Changes:
- Applies a temporary native timeout during reset and autocommit configuration, restoring it after successful checkout.
- Opens a replacement after failed validation instead of probing every stale idle connection.
- Adds stalled-socket and isolation-reset regressions, adjusts exception mocks, and documents timeout behavior.
| File | Description |
|---|---|
tests/test_009_pooling.py |
Adds stalled-checkout, timeout-restoration, and SQL isolation-reset coverage. |
tests/test_006_exceptions.py |
Injects close failures after connection construction. |
README.md |
Documents checkout timeout selection and limitations. |
mssql_python/pybind/connection/connection.h |
Extends reset to accept timeout and autocommit settings. |
mssql_python/pybind/connection/connection.cpp |
Applies and restores the temporary native timeout. |
mssql_python/pybind/connection/connection_pool.cpp |
Selects validation settings and replaces failed candidates directly. |
mssql_python/connection.py |
Passes autocommit into native checkout and documents timeout semantics. |
Review effort: Balanced
Read SQL_ATTR_CONNECTION_TIMEOUT through a private native accessor and check zero and nonzero values on the same reused SQL Server session. Mutation tests reject omitted restoration and unconditional zero. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/pybind/connection/connection.cppLines 937-947 937 SQLULEN ConnectionHandle::getConnectionTimeout() const {
938 if (!_conn) {
939 ThrowStdException("Connection object is not initialized");
940 }
! 941 SQLULEN timeout = 0;
! 942 const auto& handle = _conn->getDbcHandle();
! 943 SQLRETURN ret = SQLGetConnectAttr_ptr(handle->get(), SQL_ATTR_CONNECTION_TIMEOUT,
944 &timeout, 0, nullptr);
945 if (!SQL_SUCCEEDED(ret)) {
946 ErrorInfo error = SQLCheckError_Wrap(SQL_HANDLE_DBC, handle, ret);
947 ThrowStdException("Failed to read connection timeout: " + error.ddbcErrorMsg);Lines 946-955 946 ErrorInfo error = SQLCheckError_Wrap(SQL_HANDLE_DBC, handle, ret);
947 ThrowStdException("Failed to read connection timeout: " + error.ddbcErrorMsg);
948 }
949 return timeout;
! 950 }
! 951
952 py::object ConnectionHandle::getInfo(SQLUSMALLINT infoType) const {
953 PERF_TIMER("ConnectionHandle::getInfo");
954 if (!_conn) {
955 ThrowStdException("Connection object is not initialized");mssql_python/pybind/connection/connection_pool.cppLines 186-197 186 // expiry) before the threshold, so the safe-reuse path above
187 // is taken in practice. We favor never handing out a token
188 // that may expire mid-query over avoiding the churn.
189 candidate->setTokenExpiry(fresh_expiry);
! 190 if (candidate->isAlive()) {
! 191 validation_attempted = true;
! 192 reuse_candidate = candidate->reset(validation_timeout, autocommit);
! 193 }
194 if (!reuse_candidate) {
195 // The token is still valid but the socket is dead
196 // (isAlive()/reset() failed). We already minted the
197 // fresh attrs, so carry them forward and let Phase 3Lines 269-280 269 ++_current_size;
270 needs_connect = true;
271 break;
272 }
! 273 if (validation_attempted) {
! 274 throw std::runtime_error(
! 275 "ConnectionPool::acquire: pool size limit reached");
! 276 }
277 // A concurrent acquire reserved the slot freed by token rotation.
278 // Retry token comparison with another idle candidate.
279 }
280 }📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.performance_counter.hpp: 0.7%
mssql_python.pybind.logger_bridge.cpp: 57.9%
mssql_python.pybind.ddbc_bindings.h: 62.6%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection_pool.cpp: 81.3%
mssql_python.pybind.connection.connection.cpp: 84.1%
mssql_python.logging.py: 86.2%
mssql_python.pooling.py: 90.1%
mssql_python.pybind.fetch_temporal.hpp: 92.1%
mssql_python.cursor.py: 92.5%🔗 Quick Links
|
Sumit Sarabhai (sumitmsft)
left a comment
Sumit Sarabhai (sumitmsft)
left a comment
There was a problem hiding this comment.
Reviewed the PR for correctness, security, reliability, performance, test coverage, repository conventions, and applicable architecture and design specifications. No actionable issues were identified. The implementation is consistent with repository standards and the applicable approved design requirements.
Recommendation: Approve
Thanks for documenting the above validation results, including the initial exit-code-139 run (. Could you please share the exact scenario/command that triggered it and whether it reproduces under the same conditions? If it occurs again, a native backtrace or core dump would help identify the cause. One additional recovery scenario worth covering is:
In this case, replacement creation throws before the normal discarded-connection cleanup loop, so cleanup relies on the exception/destructor path. Could we add coverage confirming that the caller receives a normal connection exception, the process does not crash, and pool capacity remains available for subsequent successful connections? This scenario is not known to have caused the reported crash; it would strengthen coverage of the changed recovery path. I have not identified a memory-safety defect from source inspection. |
There was a problem hiding this comment.
🔵 Needs a closer look
The unexplained exit-139 crash during native packet-drop testing requires maintainer investigation and supported-platform validation, as isolated passing retries do not establish its cause or resolution.
No open issues 🎉
Review effort: Balanced
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
The unexplained exit-139 crash during packet-drop testing and outstanding Windows/macOS validation require runtime investigation of native timeout and cleanup behavior that this source-only review cannot provide.
No open issues 🎉
Review effort: Balanced
Work Item / Issue Reference
Summary
Bound pooled connection reset and autocommit configuration using a temporary native connection timeout. Use the positive login timeout (explicit attrs_before takes precedence), or a 15-second validation default, and restore the previous connection timeout on success. After validation fails, open a replacement rather than probing every stale idle socket sequentially. Preserve isolation/transaction cleanup and document that cancellation, cleanup, and replacement login can add time: this is not an end-to-end connect deadline.
Add stalled-socket regressions covering both autocommit modes, one/three stale sockets, timeout precedence, the finite default, and timeout restoration. Extend isolation reset coverage to SQL SET statements and adjust close-error mocks to inject failures after construction.
Validation (Linux x64, Python 3.13, SQL Server 2022):
bash build.shand subsequentcmake --build /work/mssql_python/pybind/build --config Release -j2: passed; rebuilt native extension used for testing.python -m pytest -v --tb=short: 5,537 passed, 167 skipped, 42 deselected. Linux test copy used LF shell scripts to avoid Windows checkout CRLF failures.python -m pre_commit run black-check --all-files --hook-stage pre-push: passed.Fixes: #854