Skip to content

FIX: forward connection timeout to bulkcopy pycore connection - #650

Merged
bewithgaurav merged 7 commits into
mainfrom
bewithgaurav/fix-626-bulkcopy-connect-timeout
Jul 13, 2026
Merged

FIX: forward connection timeout to bulkcopy pycore connection#650
bewithgaurav merged 7 commits into
mainfrom
bewithgaurav/fix-626-bulkcopy-connect-timeout

Conversation

@bewithgaurav

Copy link
Copy Markdown
Collaborator

Work Item / Issue Reference

GitHub Issue: #626


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.

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-actions github-actions Bot added the pr-size: small Minimal code update label Jun 29, 2026
Comment thread tests/test_020_bulkcopy_auth_cleanup.py Dismissed
Comment thread tests/test_020_bulkcopy_auth_cleanup.py Dismissed
Comment thread tests/test_020_bulkcopy_auth_cleanup.py Dismissed
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>
@github-actions

github-actions Bot commented Jun 30, 2026

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

100%


🎯 Overall Coverage

80%


📈 Total Lines Covered: 6743 out of 8343
📁 Project: mssql-python


Diff Coverage

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

No 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

⚙️ Build Summary 📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

@bewithgaurav
bewithgaurav marked this pull request as ready for review July 8, 2026 08:37
Copilot AI review requested due to automatic review settings July 8, 2026 08:37

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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._timeout into the bulkcopy pycore connection context as connect_timeout when Cursor._timeout > 0.
  • Keep timeout=0 as “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 thread mssql_python/cursor.py Outdated
Copilot AI and others added 2 commits July 8, 2026 14:21
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-actions github-actions Bot added pr-size: medium Moderate update size and removed pr-size: small Minimal code update labels Jul 8, 2026
Comment thread tests/test_020_bulkcopy_auth_cleanup.py Dismissed
Comment thread tests/test_020_bulkcopy_auth_cleanup.py Dismissed
@bewithgaurav
bewithgaurav merged commit d94debd into main Jul 13, 2026
29 of 30 checks passed
bewithgaurav added a commit that referenced this pull request Jul 14, 2026
Pick up 1.11.0 release, context-manager transaction (#639), bulkcopy timeout (#650), py-core 0.1.6, and macOS dylib config (#661). No profiler conflicts; auto-merge clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
@subrata-ms subrata-ms mentioned this pull request Jul 24, 2026
subrata-ms added a commit that referenced this pull request Jul 24, 2026
### Work Item / Issue Reference

>
[AB#46627](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/46627)

-------------------------------------------------------------------
### Summary

Release mssql-python v1.12.0.

Version bump to 1.12.0. Updates `mssql_python/__init__.py`, `setup.py`,
and `PyPI_Description.md`. Bundled `mssql_py_core` bumped from 0.1.6 to
0.1.7.

#### Enhancements

- **Standalone `mssql-python-odbc` package** — ODBC driver binaries are
now also published as a separate, pure-data companion package
`mssql-python-odbc` (import name `mssql_python_odbc`, version 18.6.2).
`mssql-python` declares it in `install_requires` and prefers the
external package at import time, falling back to the still-bundled
`libs/` tree so existing installs keep working (#663, #687).

#### Bug Fixes

- **Bulk copy connection timeout** — `cursor.bulkcopy()` now forwards
the cursor's connection timeout (from `connect(timeout=X)`) into the
underlying `mssql_py_core` connection, instead of always using the
hardcoded 15s default (#650, issue #626).
- **Bulk copy of custom CLR UDT columns** — Fixed `Protocol Error:
Unsupported TDS type for bulk copy: 0xF0` on `cursor.bulkcopy()` into
custom (non-spatial) CLR UDT columns; UDT columns are now mapped to
`varbinary(max)` on the wire (#688, via `mssql_py_core` 0.1.7, issue
#667).

#### Version Bump

- `mssql_python/__init__.py`: `__version__ = "1.12.0"`
- `setup.py`: `version="1.12.0"`
- `PyPI_Description.md`: `## What's new in v1.12.0` section refreshed.
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.

6 participants