FIX: forward connection timeout to bulkcopy pycore connection#650
Merged
bewithgaurav merged 7 commits intoJul 13, 2026
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>
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.
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
saurabh500
approved these changes
Jul 9, 2026
gargsaumya
approved these changes
Jul 10, 2026
gargsaumya
added a commit
that referenced
this pull request
Jul 14, 2026
…sign D13) Remove the keyword-only '*' from Cursor.bulkcopy_arrow (cursor.py + stub) so batch_size/timeout match bulkcopy's signature per the finalized design (D13/section 2). Also set cursor._timeout in the test mock-cursor helper (main's #650 added self._timeout to the shared _build_pycore_context) and add a positional-args regression test.
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.