Skip to content

Remove ResultSet async_trait boxing - #291

Open
Saurabh Singh (saurabh500) wants to merge 6 commits into
dev/saurabh/remove-row-path-async-traitsfrom
dev/saurabh/native-resultset-futures
Open

Remove ResultSet async_trait boxing#291
Saurabh Singh (saurabh500) wants to merge 6 commits into
dev/saurabh/remove-row-path-async-traitsfrom
dev/saurabh/native-resultset-futures

Conversation

@saurabh500

@saurabh500 Saurabh Singh (saurabh500) commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Description

This focused follow-up removes #[async_trait] from the public ResultSet trait and impl ResultSet for TdsClient. The async methods now use native RPITIT futures with explicit + Send; the concrete implementation returns the existing native client futures directly, avoiding per-call trait-future boxing while preserving row cleanup and end-of-set behavior.

This PR is stacked on #286 (dev/saurabh/remove-row-path-async-traits) as the new top layer of native stack #287. It does not change TdsTransport, TdsTokenStreamReader, or transport dispatch.

Public API impact

ResultSet is intentionally no longer dyn-compatible. This is a breaking object-safety change for dyn ResultSet consumers and for external implementations using #[async_trait]. Existing concrete call sites retain the same .method(...).await source shape; implementations must migrate to native Send futures. Repository search found no trait-object consumers.

Measurements

A temporary in-crate release harness (removed before commit) compared native ResultSet::next_row with an explicit Pin<Box<dyn Future + Send>> wrapper around the same native method. Each independent process used a paired/interleaved ABBA sequence over 500,000 production-shaped rows with 48 INT columns. Across seven processes, the boxed path was 1.52%–4.64% slower, with a 3.63% median. The range is reported as measured rather than excluding noisy runs.

Current debug future sizes are:

  • ResultSet::next_row: 4,056 B
  • ResultSet::next_row_into: 4,032 B
  • ResultSet::close: 4,376 B

Both per-row futures are guarded under the existing 4,096 B hot-path budget. close is larger, but it is not a per-row operation, so this change documents the effect instead of adding broad boxing.

Validation

  • cargo bfmt — passed
  • cargo bclippy — passed
  • cargo btest — 2,410 passed, 366 environment failures, 11 skipped; exactly matches the current lower-layer baseline (7 missing certificate-fixture tests and 359 live-SQL tests)
  • .\scripts\bfmt.ps1 — passed
  • .\scripts\bclippy.ps1 — passed, including excluded mssql-py-core
  • Focused ResultSet future-size, idempotent end-of-set, and prepared-capture cleanup tests — passed
  • Workspace consumers (mssql-js, CLI, ODBC, and benchmarks) compile through the all-features/all-targets Clippy pass; no fuzz target references ResultSet

Related Issues

Related to #265.

Checklist

  • cargo bfmt passes
  • cargo bclippy passes
  • cargo btest passes (blocked only by the 366 environment failures matching the lower-layer baseline)
  • New/changed functionality has tests
  • Public API changes are documented

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 89c55fda-9c7a-4785-856d-e7e81c45f34f

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

Removes async_trait boxing from ResultSet, using native statically dispatched Send futures while preserving existing behavior.

Changes:

  • Replaces boxed async trait methods with RPITIT futures.
  • Delegates directly to native TdsClient futures.
  • Adds future-size and behavioral regression coverage.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 89c55fda-9c7a-4785-856d-e7e81c45f34f
@saurabh500
Saurabh Singh (saurabh500) marked this pull request as ready for review August 14, 2026 17:32
@saurabh500
Saurabh Singh (saurabh500) requested a review from a team as a code owner August 14, 2026 17:32

@David-Engel David Engel (David-Engel) 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.

Summary

Converts ResultSet from #[async_trait] to native RPITIT futures with explicit + Send, and makes the TdsClient impl forward the existing native inherent futures. The change is correct and behavior-preserving. No blocking issues.

Reviewed on a dedicated worktree at head 418c3ab5, diffed against the stacked base dev/saurabh/remove-row-path-async-traits. Verified locally:

  • cargo clippy --workspace --all-features --all-targets — clean
  • mssql-py-core (excluded from bclippy) — cargo check --all-targets clean
  • cargo test -p mssql-tds --lib — 1,659 passed, only the 4 known certificate-fixture failures
  • cargo test -p mssql-tds --doc — 9 failures, all pre-existing private-module doctests unrelated to this PR

Claims I confirmed

Behavior parity on next_row. Dropping the outer maybe_has_unread_rows() guard and abort_pending_prepare_capture() is safe: maybe_has_unread_rows() is exactly !current_result_set_has_been_read_till_end, which the inherent next_row_into already short-circuits, and every error path in that method calls abort_pending_prepare_capture() before returning. The two capture-abort tests still cover it.

No trait-object consumers. Zero dyn ResultSet, a single impl ResultSet for TdsClient, and all consumers (mssql-js, mssql-odbc, mssql-py-core, mssql-tds-cli, benches, integration tests) go through concrete TdsClient. Method-call syntax (client.next_row().await) still compiles, so the "same source shape" claim holds.

CI note (not a code finding)

coverage-report failed with "No build found for PR #291 after 30 attempts", and the ADO mssql-rs Pull request validation check is skipping — both because the PR targets the stacked branch instead of main. This is not the usual 75-minute artifact-timeout flake, and a re-run won't fix it. Net effect: this change gets no ADO validation and no 85% diff-coverage signal on its own. Worth confirming the full stack (#286 -> #287 -> this) is validated against main before merge.

Four non-blocking comments inline.

Comment thread mssql-tds/src/connection/tds_client.rs
Comment thread mssql-tds/src/connection/tds_client.rs
Comment thread mssql-tds/src/connection/tds_client.rs
Comment thread mssql-tds/src/connection/tds_client.rs
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 89c55fda-9c7a-4785-856d-e7e81c45f34f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 89c55fda-9c7a-4785-856d-e7e81c45f34f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 89c55fda-9c7a-4785-856d-e7e81c45f34f
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 89c55fda-9c7a-4785-856d-e7e81c45f34f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants