Sans-I/O TDS core (3/N): invert token/decoder to a sync step() core - #194
Conversation
Introduce a synchronous, I/O-free read core on PacketBuffer and drive it from the async reader shell, so the suspension point of the token/decoder read path collapses to a single well-defined seam. - Add PacketBuffer::ensure(n) -> Result<(), NeedBytes>: an atomic, non-consuming guard that reports the byte shortfall. Paired with the existing atomic take_* accessors, a fixed-width read never advances the position on a short buffer and is always safe to re-drive after refill. - Invert both TdsPacketReader impls (test PacketReader and production NetworkTransport) into thin async driver loops over ensure(): on NeedBytes, refill one packet and retry. Trait signatures are unchanged, so TdsClient and the frozen public API are untouched. - Guard the driver against the L2 spin class: every refill must expose at least one new byte or the loop fails loudly (debug_assert) / returns a ProtocolError, never hangs on a zero-payload packet. - Delete the read_plp_bytes collect-all duplicate and route eager PLP materialization through the shared PlpChunkStreamReader via a thin collect_plp helper, removing ~90 lines of duplicated chunk framing. - Cover empty-input and exact-packet-boundary termination plus the ensure shortfall/atomicity invariants with unit tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
Restore direct coverage of the sync-core positional accessors that L2's buffer consolidation left only indirectly exercised. These make take_*/ ensure the tested contract of the L3 read core. - take_* roundtrip + exact position advance across every fixed width, plus a breadth test for the signed/float/40-bit decoders. - ensure(n)/take_* atomicity: a short read is a no-op (cursor unchanged, bytes intact, safe to re-drive); an exact-boundary read drains and resets the cursor. - skip_available/copy_out advance from the current position and saturate at what is buffered, never over-reading. - Over-consume bounds guard: a take wider than available is rejected outright without corrupting the cursor, protecting the L3 loop-termination invariant. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e2c378f8-3ba1-4b48-9ebe-5a4ea2bd2761
a95c64b to
2fd689c
Compare
There was a problem hiding this comment.
Pull request overview
Introduces the synchronous Sans-I/O buffer guard and adapts async readers around it while consolidating PLP decoding.
Changes:
- Adds atomic
PacketBuffer::ensure. - Converts packet readers into refill-driven loops.
- Consolidates eager PLP collection through the streaming reader.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
mssql-tds/src/io/packet_reader.rs |
Adds refill loops and termination tests. |
mssql-tds/src/io/packet_buffer.rs |
Adds NeedBytes, ensure, and accessor tests. |
mssql-tds/src/datatypes/decoder.rs |
Deduplicates PLP materialization. |
mssql-tds/src/connection/transport/network_transport.rs |
Adopts synchronous guards in production reads. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // one new byte. Without this guard a zero-payload packet | ||
| // would spin the driver forever, so fail loudly instead. | ||
| let progressed = self.tds_read_buffer.available() > before; | ||
| debug_assert!(progressed, "TDS refill made no forward progress"); |
| // one new byte. If it does not, re-driving the read would | ||
| // spin forever, so fail loudly instead of hanging. | ||
| let progressed = self.buffer.available() > before; | ||
| debug_assert!(progressed, "TDS refill made no forward progress"); |
| .await?; | ||
| offset += chunk_size_read; | ||
| chunk_len = reader.read_uint32().await? as usize; | ||
| let mut collected = Vec::new(); |
| offset += chunk_size_read; | ||
| chunk_len = reader.read_uint32().await? as usize; | ||
| let mut collected = Vec::new(); | ||
| let mut chunk = [0u8; COLLECT_CHUNK]; |
|
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. |
Layer 3 of the green-at-every-step sans-I/O stack
Base:
dev/saurabh/sans-io-layer2-transport(#191). Public API is frozen —mssql-odbcandmssql-py-corebuild unchanged.What this lands
This slice inverts the reader seam to a synchronous, I/O-free core while the async shell remains the only public driver. It is the foundation the parser-level
step()core builds on next.PacketBuffer::ensure(n) -> Result<(), NeedBytes>— an atomic, non-consuming guard reporting the exact byte shortfall. Paired with the existing atomictake_*accessors, a fixed-width read never advances on a short buffer and is always safe to re-drive after a refill.TdsPacketReaderimpls (testPacketReader+ productionNetworkTransport) are now thin async driver loops:loop { match buf.ensure(n) { Ok => take, NeedBytes => refill().await } }. Trait signatures are unchanged, soTdsClientand the frozen API are untouched. Both#[cfg(fuzzing)]/#[cfg(not(fuzzing))]trait defs are maintained.debug_assert) / returns aProtocolError— never spins on a zero-payload packet.read_plp_bytescollect-all duplicate (~90 lines of copied chunk framing) and routed eager materialization through the sharedPlpChunkStreamReadervia a thincollect_plphelper. All size/chunk guards now live in one place.ensureshortfall/atomicity invariants, plus directPacketBufferaccessor + over-consume bounds-guard coverage.Scope note (honest labeling)
The reader-seam inversion + sync
NeedBytescore + PLP dedup are landed here as a self-contained, low-risk layer. The parser-level column-atomicstep()core (re-drivingdecode_row_columns/ NBCROW / PLP chunk parsing over the sync buffer) is L4 — kept separate so every step stays green, and it is the layer that actually removes the async tax.DecodePolicy/BatchExitenums were intentionally not introduced: no existingbool/unreachable!in the touched code maps cleanly to them, so adding them would be noise per the repo's no-AI-slop charter.Validation (green)
cargo build✅cargo bclippy(warnings = errors) ✅cargo bfmt✅cargo btest(nextest): lib green except the 7 pre-existingtest_certificates/win_tlscert-fixture failures (integration tests needing a live SQL Server are environmental); failing set verified byte-for-byte identical to the base branch. ✅cargo build --manifest-path mssql-py-core/Cargo.toml(outside workspace) ✅LOC delta reported on the final rebased commit.