[py-core] Async Connection Foundation - #288
Conversation
Adds a Python-side 'timeout' property (default 0 = no timeout, per pyodbc/ODBC SQL_ATTR_QUERY_TIMEOUT convention) plumbed through cursor() so a future PyAsyncCursor::execute (US 46961) can pick it up as the per-call default. Pure state; the setter performs no I/O. Tests cover default, roundtrip, and negative-value rejection.
Both derive from tds_client Option state (None after close(). take()). - closed: bool property, matches async ecosystem convention (aioodbc, asyncpg, aiosqlite, mssql-python sync). - is_connected(): sync-parity with PyCoreConnection; inverse of closed. No I/O; both are pure O(1) reads. Tests cover live/closed/idempotent.
Enables 'async with await connect(ctx) as conn:' — deterministic cleanup in async code where __del__ cannot await close_connection(). __aenter__ resolves to self; __aexit__ delegates to close() and never suppresses exceptions. Tests cover happy path, same-object identity, and exception propagation across close.
…ection - __repr__ returns 'PyAsyncConnection(connected|closed)' matching sync connection's style; returns &'static str to avoid allocation. - map_tds_error(op, user_msg, e) centralizes the TDS->PyErr conversion. TODO(User Story 47181) now lives at a single site instead of three, so the future switch to DB-API-compliant exceptions is one edit. - Trace format and user-facing error messages are byte-preserved. Tests: two integration tests cover __repr__ live/closed transitions.
- Merge overlapping lifecycle, timeout, and __repr__ tests (5 -> 3). - Drop test_close_is_idempotent (superseded). - Rename commit/rollback/close tests to describe the actual assertion. - Add three tests for the previously uncovered cursor() code path: cursor returns PyAsyncCursor, cursor after close raises, multiple cursors from one connection allowed per module invariant. 19 -> 17 tests; local run passes 17/17 in <1s against dockerized SQL Server 2025.
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
🔗 Quick Links |
There was a problem hiding this comment.
Pull request overview
Builds the foundational Python async connection API for the TDS backend.
Changes:
- Adds lifecycle, timeout, context-manager, logging, and error-mapping support.
- Propagates connection timeout defaults to new async cursors.
- Expands async connection integration tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mssql-py-core/src/async_connection.rs |
Implements the expanded async connection API. |
mssql-py-core/src/async_cursor.rs |
Stores inherited query timeout state. |
mssql-py-core/tests/test_async_connection.py |
Tests lifecycle, timeout, context-manager, and cursor behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // `DefaultGuard` is `!Send`, so its coverage ends when this method returns. | ||
| let _guard = python_logger | ||
| .map(|logger| scoped_tracing_bridge(Arc::new(logger.clone().unbind()), file!())); |
| //! `TdsClient`, and one TDS wire session. | ||
| //! | ||
| //! [`FutureWarning`]: https://docs.python.org/3/library/exceptions.html#FutureWarning | ||
| //! Invariant: one async connection ↔ one async cursor ↔ one `TdsClient`. |
David Engel (David-Engel)
left a comment
David Engel (David-Engel)
left a comment
There was a problem hiding this comment.
Summary
Incremental enhancement of the preview PyAsyncConnection surface in mssql-py-core: adds .timeout, .closed, is_connected(), __repr__, __aenter__/__aexit__, a python_logger param on connect, and factors TDS-to-Python error mapping into map_tds_error. The Rust is clean and idiomatic, the test consolidation is a genuine improvement (the dropped test_close_is_idempotent is subsumed by test_lifecycle_state_reflects_closed_and_is_connected), and no coverage is lost. CI is green and diff coverage is 100%.
One blocking item on the new python_logger parameter (inline), plus a few suggestions and nits. Details are threaded on the relevant lines.
Not tied to a diff line
Suggestion - docstring trim dropped user-visible contract. The rustdoc on #[pymethods] becomes Python __doc__. commit/rollback lost "Raises RuntimeError synchronously if the connection has already been closed", and close() lost the "resolves to None" statement that you have a dedicated regression test for. I am on board with cutting the prose, but those are contract, not commentary - worth keeping one line each.
Nit - PR description hygiene before merge:
- The auto-generated summary is duplicated above the hand-written
## Description. Work item: **[TASK 46956]** - link the ADO work item here.is leftover placeholder text, and 46956 is not among the three work items linked under Related Issues (46959 / 46960 / 47185) even though all eight commits reference it.cargo btest, "Full validation pipeline", and the entire bottom Checklist are unchecked, but CI is green - please tick them (per.github/PULL_REQUEST_TEMPLATE.md).- Title says "Async Connection Foundation", but the foundation landed in #206; this PR enhances it. Something like "[py-core] Async connection lifecycle, timeout, and context-manager support" would read truer.
| // `DefaultGuard` is `!Send`, so its coverage ends when this method returns. | ||
| let _guard = python_logger | ||
| .map(|logger| scoped_tracing_bridge(Arc::new(logger.clone().unbind()), file!())); |
There was a problem hiding this comment.
Blocking: this bridge does not cover any of the async work, including the connect-failure log.
scoped_tracing_bridge calls tracing::subscriber::set_default, which is thread-local and is undone when the guard drops at the end of connect. The future_into_py body runs later, on a Tokio worker thread. So of the events in connect, the Python logger receives only the four synchronous pre-flight info! lines, and never receives:
"connection established"(L123)map_tds_error("connect", ...)'stracing::error!(L50) - the handshake-failure diagnostic, which is the whole reason a caller passes a logger
This is the opposite of the sync path it is modeled on: PyCoreConnection::new uses runtime.block_on on the same thread, so its tracing::error!("Failed to connect to SQL Server: {}", e) is inside the guard. Separately, close/commit/rollback/cursor take no logger and are never bridged at all, so the connection is unlogged for its entire lifetime after connect returns.
Concrete fix - propagate a Dispatch across the await point instead of a thread-local guard. Add alongside the existing helpers in python_logger_adapter.rs:
pub fn tracing_bridge_dispatch(logger: Arc<Py<PyAny>>, module_name: &'static str) -> tracing::Dispatch {
let layer = PythonLoggerLayer::new(logger, module_name)
.with_filter(filter_fn(|m| m.target().starts_with("mssql_py_core")));
tracing::Dispatch::new(Registry::default().with(layer))
}then here:
use tracing::instrument::WithSubscriber;
let dispatch = python_logger
.map(|l| tracing_bridge_dispatch(Arc::new(l.clone().unbind()), file!()));
// ... sync tracing calls stay inside a set_default guard as today ...
pyo3_async_runtimes::tokio::future_into_py(py, async move {
let fut = async move { /* existing body */ };
match dispatch { Some(d) => fut.with_subscriber(d).await, None => fut.await }
})Dispatch is Clone + Send + Sync, so it can also be stored on the pyclass and reused by close/commit/rollback - which is what makes the parameter actually useful.
If that is too much for this PR, I would accept: drop the python_logger param for now, or keep it and (a) state the limitation in the Python-visible docstring rather than only a code comment, and (b) file a follow-up work item. What I would rather not ship is a python_logger= that silently swallows the failure path.
Supporting point: there is no test that passes a python_logger. A test asserting which records reach py_core_log for both a successful and a failed connect would have caught this, and is the right regression guard for whichever fix you pick.
|
|
||
| emit_preview_warning(py)?; | ||
|
|
||
| tracing::info!("PyAsyncConnection::connect: initiating async connection"); |
There was a problem hiding this comment.
Nit: "initiating async connection" is immediately followed by "extracting client context" at the same level - the first adds nothing. Also, the sync path uses debug! for the dict conversion; info! for every step is chatty on a per-connection path.
| /// Set the default query timeout (seconds) for future cursors. Existing | ||
| /// cursors and in-flight queries are unaffected. Negative values are | ||
| /// rejected by PyO3's `u32` extractor (`OverflowError`). | ||
| #[setter] | ||
| fn set_timeout(&mut self, value: u32) { | ||
| tracing::info!( | ||
| "PyAsyncConnection::set_timeout: default query timeout set to {}s", | ||
| value | ||
| ); | ||
| self.default_query_timeout = value; | ||
| } |
There was a problem hiding this comment.
Suggestion: negative values raise OverflowError here, but mssql_python.Connection.timeout's setter raises ValueError("Timeout cannot be negative") (and TypeError for non-int). test_timeout_setter_rejects_negative locks OverflowError in.
If the Python wrapper will always sit on top, this is fine - but worth a one-line note in the docstring so nobody is surprised when the two surfaces disagree. Otherwise take i64 and raise ValueError explicitly for parity.
| fn __aenter__<'py>(slf: Py<Self>, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> { | ||
| pyo3_async_runtimes::tokio::future_into_py(py, async move { Ok(slf) }) | ||
| } |
There was a problem hiding this comment.
Suggestion: cursor(), commit(), and rollback() all raise RuntimeError("Connection is closed"), but async with closed_conn: succeeds and then no-ops on exit. Suggest matching the rest of the surface:
fn __aenter__<'py>(slf: Py<Self>, py: Python<'py>) -> PyResult<Bound<'py, PyAny>> {
if slf.borrow(py).tds_client.is_none() {
return Err(PyRuntimeError::new_err("Connection is closed"));
}
pyo3_async_runtimes::tokio::future_into_py(py, async move { Ok(slf) })
}| /// Sync per DB-API 2.0. A second cursor is allowed; both share the same | ||
| /// TDS session and serialize on the same async mutex. |
There was a problem hiding this comment.
Suggestion: this docstring is now a fragment that says neither what cursor() returns nor that it raises RuntimeError when the connection is closed - both of which are contract that shows up in Python help(). Worth one line back:
/// Create an async cursor. Sync per DB-API 2.0. Raises `RuntimeError` if the
/// connection is closed. A second cursor is allowed; both share the same TDS
/// session and serialize on the same async mutex.
| /// Snapshot of the parent connection's default query timeout at | ||
| /// `cursor()` time (`0` = no timeout). Applied by the future `execute` | ||
| /// path unless overridden per-call. | ||
| #[allow(dead_code)] // Consumed by upcoming async execute API. | ||
| default_query_timeout: u32, |
There was a problem hiding this comment.
Suggestion: this field is private with #[allow(dead_code)] and has no getter, so nothing verifies that cursor() actually forwards the value. test_timeout_default_and_setter_roundtrip only exercises the connection property, and the 100% diff coverage is line coverage - PyAsyncCursor::new(client, self.default_query_timeout) executes, but always with 0.
Smallest fix - add a getter (also drops one #[allow(dead_code)]):
#[pymethods]
impl PyAsyncCursor {
/// Query timeout (seconds) snapshotted from the parent connection. `0` = no timeout.
#[getter]
fn timeout(&self) -> u32 { self.default_query_timeout }
}and assert it:
conn.timeout = 30
assert conn.cursor().timeout == 30
This pull request significantly enhances the
PyAsyncConnectionasync API inmssql-py-core, focusing on improved usability, lifecycle management, and future extensibility. Key changes include the addition of connection state and timeout properties, async context manager support, improved error mapping, and updated cursor construction. The test suite has also been expanded and clarified to cover these new behaviors.Major enhancements and fixes:
Connection lifecycle and usability improvements:
timeoutproperty (getter/setter) toPyAsyncConnectionfor default query timeout management; this value is now passed to each new cursor. Also addedclosedandis_connectedproperties for explicit connection state checks, and a__repr__for clear string representation. [1] [2]__aenter__,__aexit__) forPyAsyncConnection, enablingasync withusage and ensuring connections are properly closed.Error handling and logging:
map_tds_errorhelper to consistently map TDS errors to Python exceptions with operation context, improving debugging and future DB-API compliance. [1] [2]Cursor and timeout propagation:
PyAsyncCursorto accept and store the default query timeout from the parent connection, preparing for per-query timeout support. [1] [2]Test coverage and clarity:
These changes collectively make the async connection API more robust, user-friendly, and ready for future enhancements.
Description
Summary
Foundational async query execution surface:
PyAsyncConnectioninmssql-py-core.Preview API — first use emits
FutureWarning.Work item: [TASK 46956] — link the ADO work item here.
What's in
connect:python_loggerparam + tracingclose: entry + completion tracing.timeoutdefault query timeout getter/setter (0 = no timeout).closedproperty +is_connected()method__aenter__/__aexit__forasync with__repr__+map_tds_errorhelper (single TODO(47181) site)cursor()code path coverageAsync surface (PyAsyncConnection)
connect(client_context_dict, python_logger=None)classmethod, awaitableclose()awaitable, idempotent, log-and-swallow shutdown errorscommit()/rollback()awaitablescursor()sync method →PyAsyncCursor(scaffold for US 46961).timeoutu32 getter/setter (per-cursor query-timeout default).closedbool property,is_connected()bool method__aenter__/__aexit__forasync with await connect(ctx) as conn:__repr__returns"PyAsyncConnection(connected|closed)"Zero touches to sync
connection.rsexcept a singledict_to_client_contextvisibility bump topub(crate)(reused for context parsing).DB-API 2.0 status
Method-complete against PEP 249 §4.1 (close/commit/rollback/cursor).
Optional extensions (
Connection.Erroret al.) blocked on User Story 47181 (DB-API exception hierarchy).autocommitblocked on mssql-tds primitive (SET IMPLICIT_TRANSACTIONS).Testing
17 tests in
mssql-py-core/tests/test_async_connection.py:Local run: 17/17 passing in <1s against dockerized SQL Server 2025.
Validation
cargo bfmtcargo bclippycargo btestFollow-ups (out of scope)
PyAsyncCursor.execute/fetch*/closemap_tds_errorfactored to a single siteapilevel,paramstyle,threadsafety).autocommit— needs mssql-tdsset_implicit_transactionsprimitive firstRelated Issues
ADO work item: https://sqlclientdrivers.visualstudio.com/mssql-python/_workitems/edit/46959, https://sqlclientdrivers.visualstudio.com/mssql-python/_workitems/edit/46960, https://sqlclientdrivers.visualstudio.com/mssql-python/_workitems/edit/47185
Checklist
cargo bfmtpassescargo bclippypassescargo btestpasses