FIX: forward connection timeout to bulkcopy pycore connection#650
Open
bewithgaurav wants to merge 7 commits into
Open
FIX: forward connection timeout to bulkcopy pycore connection#650bewithgaurav wants to merge 7 commits into
bewithgaurav wants to merge 7 commits into
Conversation
cursor.bulkcopy() opens a separate connection through mssql-py-core, which defaulted to a hardcoded 15s connect timeout with no way to override it. forward the cursor's query timeout (connect(timeout=X)) into pycore's connect_timeout when set, so the same limit applies to the bulk copy connection. 0 stays a no-override, leaving pycore on its default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| def test_positive_timeout_forwarded(self, mock_logger): | ||
| """cursor._timeout > 0 ⇒ connect_timeout reaches pycore, overriding 15s.""" | ||
| mock_logger.is_debug_enabled = False | ||
| cursor = _make_cursor("Server=localhost;Database=testdb;UID=sa;PWD=pwd", None) |
| def test_zero_timeout_not_forwarded(self, mock_logger): | ||
| """cursor._timeout == 0 ⇒ no override, pycore keeps its default.""" | ||
| mock_logger.is_debug_enabled = False | ||
| cursor = _make_cursor("Server=localhost;Database=testdb;UID=sa;PWD=pwd", None) |
| def test_uses_cursor_snapshot_not_live_connection(self, mock_logger): | ||
| """timeout is the cursor snapshot; later connection changes don't apply.""" | ||
| mock_logger.is_debug_enabled = False | ||
| cursor = _make_cursor("Server=localhost;Database=testdb;UID=sa;PWD=pwd", None) |
the msi-client-id bulkcopy regression test builds a Cursor via __new__ without _timeout. _bulkcopy now reads self._timeout to forward connect_timeout (issue #626), so the bare mock raised AttributeError. set _timeout=0 to match a real cursor, same as the other bulkcopy mocks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. 📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.connection.connection.cpp: 76.2%
mssql_python.pybind.ddbc_bindings.cpp: 76.2%
mssql_python.__init__.py: 77.3%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.connection.py: 83.6%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #626 where cursor.bulkcopy() opens a separate mssql-py-core connection that previously always used pycore’s compiled-in default connect timeout (15s), ignoring the Python-side timeout set via connect(timeout=...). The change forwards the cursor’s snapshot timeout into the pycore context as connect_timeout (only when the cursor timeout is a positive integer), and adds regression tests for the forwarding and non-forwarding cases.
Changes:
- Forward
Cursor._timeoutinto the bulkcopy pycore connection context asconnect_timeoutwhenCursor._timeout > 0. - Keep
timeout=0as “no override” so pycore’s default connect timeout remains in effect. - Add tests to validate positive forwarding, zero behavior, and that the cursor snapshot is used (not later connection changes).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| mssql_python/cursor.py | Adds forwarding of the cursor timeout into pycore bulkcopy connection context via connect_timeout. |
| tests/test_020_bulkcopy_auth_cleanup.py | Adds unit tests covering connect timeout forwarding behavior for the bulkcopy path. |
| tests/test_008_auth.py | Updates cursor test setup to include _timeout so bulkcopy-path tests continue to work. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2963
to
+2965
| connect_timeout = self._timeout | ||
| if type(connect_timeout) is int and connect_timeout > 0: | ||
| pycore_context["connect_timeout"] = connect_timeout |
the public Connection.timeout setter validates with isinstance(int), so it accepts IntEnum members. the earlier type(x) is int guard rejected those, so a cursor could apply the query timeout but silently fall back to py-core's 15s for the bulkcopy connect. switch to isinstance(int) and not isinstance(bool), normalise to a plain int, and keep the >0 gate: py-core's TCP attempt path turns connect_timeout=0 into a 0ms timeout that fails instantly, so 0 stays unset and py-core applies its own default. tests cover IntEnum forwarding and bool rejection. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…://github.com/microsoft/mssql-python into bewithgaurav/fix-626-bulkcopy-connect-timeout
github-actions
Bot
added
pr-size: medium
| def test_bool_timeout_not_forwarded(self, mock_logger): | ||
| """bool is a subclass of int but must not be treated as a timeout.""" | ||
| mock_logger.is_debug_enabled = False | ||
| cursor = _make_cursor("Server=localhost;Database=testdb;UID=sa;PWD=pwd", None) |
saurabh500
approved these changes
Jul 9, 2026
gargsaumya
approved these changes
Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
cursor.bulkcopy() opens a separate connection through mssql-py-core, which defaulted to a hardcoded 15s connect timeout with no way to override it from Python. I forward the cursor's query timeout (set via connect(timeout=X)) into pycore's connect_timeout when it's set, so the bulk copy connection honors the same limit. timeout=0 stays a no-override, leaving pycore on its 15s default. added tests covering the positive forward, the zero case, and that the cursor snapshot (not a later live connection change) is what's used.