Sans-I/O TDS core (L2): lift request timeout and cancellation to the receive edge - #201
Closed
Saurabh Singh (saurabh500) wants to merge 3 commits into
Conversation
The async shell applied the per-request timeout and cancellation by wrapping the whole token/row drive future in tokio::time::timeout plus a CancellationToken. That tokio-timer wrap was the last runtime coupling on the otherwise sans-I/O parse path: the only place a drive future yields Pending is the socket read, so the whole-future timer could only ever fire there. Move the concern to the ByteSource edge via a ReceiveGuard carrying an absolute deadline and the cancel token. The async TdsTokenStreamReader shell snapshots it before each drive and resets it after; AsyncByteSource::receive applies timeout_at plus run_until_cancelled around the socket read, reusing the existing error paths so timeout and cancel stay byte-identical. The shared assemble/drive bodies now only call receive(), leaving them timer- and cancel-agnostic for a future non-tokio driver. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
Prove the lifted request timeout+cancel still fire at the AsyncByteSource socket-read edge with the same TimeoutError(Elapsed) and OperationCancelledError, and never mark the connection dead on either. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 10, 2026 06:38
Copilot started reviewing on behalf of
Saurabh Singh (saurabh500)
August 10, 2026 06:39
View session
Contributor
There was a problem hiding this comment.
Pull request overview
Moves request timeout and cancellation handling from token/row drivers to the socket receive edge.
Changes:
- Adds receive guards with absolute deadlines and cancellation tokens.
- Threads guards through
NetworkTransport. - Adds deterministic timeout/cancellation tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
mssql-tds/src/io/byte_source.rs |
Implements guarded socket receives and tests. |
mssql-tds/src/connection/transport/network_transport.rs |
Applies guards around request drivers. |
mssql-tds/Cargo.toml |
Enables Tokio test clock utilities. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+85
to
+87
| /// The socket read is wrapped by the request's [`ReceiveGuard`] so timeout and | ||
| /// cancellation fire at this edge, producing the same error kinds the async | ||
| /// shell's whole-future tokio wrap did before the lift. |
Comment on lines
+1264
to
+1266
| self.receive_guard = ReceiveGuard::new(remaining_request_timeout, cancel_handle); | ||
| let token_result = drive_token_over_buffer(self, &*PARSER_REGISTRY, context).await; | ||
| self.receive_guard = ReceiveGuard::default(); |
Saurabh Singh (saurabh500)
marked this pull request as draft
August 10, 2026 06:49
Saurabh Singh (saurabh500)
marked this pull request as ready for review
August 10, 2026 07:39
Saurabh Singh (saurabh500)
marked this pull request as draft
August 10, 2026 13:28
Contributor
Author
|
Closing: the sans-I/O restructuring is not going to be productionized. The row-decode performance work that motivated much of this has been re-scoped around #247, where benchmarked spikes show the dominant win comes from a far smaller change — converting This is cleanup rather than a rejection of the analysis. The branch is deliberately not deleted, so the work remains recoverable if the direction is revisited. |
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.
Layer 2 of the sans-I/O core stack (internal refactor; no public API change)
Base:
dev/saurabh/sans-io-expose-l1-bytesource(Layer 1, PR #200).What
The async shell applied the per-request timeout + cancellation by wrapping the whole token/row drive future in
tokio::time::timeout+ aCancellationToken(the 4TdsTokenStreamReadermethods). That tokio-timer wrap was the last runtime coupling on the otherwise sans-I/O parse path.This lifts the concern to the
ByteSourceedge:pub(crate) ReceiveGuard { deadline: Option<Instant>, cancel: Option<CancellationToken> }snapshots the request's absolute deadline + cancel token.TdsTokenStreamReadershell sets the guard before each drive and resets it after; the timeout/cancel cleanup (cancel_read_stream_and_wait) is unchanged.AsyncByteSource::receiveappliestimeout_at+run_until_cancelledaround the socket read, reusing the existing error paths so timeout →TimeoutError(Elapsed)and cancel →OperationCancelledErrorstay byte-identical.assemble_tds_packet,drive_*,step_*) now only callreceive()— timer- and cancel-agnostic, ready for a future non-tokio driver.Why byte-identical
The only point any drive future yields
Pendingis the socket read (all refills funnel throughget_new_tds_packet). So today's whole-futuretimeoutcould only ever fire there; moving an absolute deadline + cancel to wrap exactly that read preserves the same total-request deadline, the same cancel, and the same error kinds/messages.Gates
bba556bfEMPTY.cargo nextest -p mssql-tds --lib: 1703 passed, 7 pre-existing baseline failures (cert/pin tests), no new failures/hangs.cargo bfmt,cargo bclippy,scripts/bfmt.ps1,scripts/bclippy.ps1all clean.Untouched: shared parse bodies, the
#[cfg(fuzzing)]oracle, and the attention-ACK / connect tokio timeouts (shell-local).