Skip to content

Sans-I/O TDS core (1/N): extract PacketBuffer - #189

Closed
Saurabh Singh (saurabh500) wants to merge 1 commit into
mainfrom
dev/saurabh/sans-io-tds-core
Closed

Sans-I/O TDS core (1/N): extract PacketBuffer#189
Saurabh Singh (saurabh500) wants to merge 1 commit into
mainfrom
dev/saurabh/sans-io-tds-core

Conversation

@saurabh500

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

Copy link
Copy Markdown
Contributor

Stack

Bottom layer of the sans-I/O mssql-tds restructure. This is layer 1 of a GitHub PR stack; higher layers build on this branch.

What this layer does

Introduces PacketBuffer — the synchronous, I/O-free half of packet reading. It owns the reassembled TDS payload bytes and serves every scalar/byte read straight from memory, knowing nothing about sockets or async. The only thing it can't do itself is obtain more bytes; when a read needs more than what's buffered, the caller (a thin I/O shell) refills via begin_refill / commit_packet and retries.

PacketReader is rewired onto PacketBuffer, so the buffer math and scalar decode now live in one place. The reader keeps only the one thing the buffer can't do — pull bytes off the socket on refill (receive_packet). The four ad-hoc buffer fields collapse to a single buffer: PacketBuffer, and the duplicated consume_bytes / do_we_have_enough_data logic is gone.

Public API is unchanged — this is a pure internal refactor that establishes the sans-I/O abstraction.

Why

The TDS protocol logic (framing, scalar decode) is pure computation over an in-memory byte buffer; the only real I/O is "the buffer is empty, give me more bytes." Making that split explicit lets a sync shell and an async shell later drive the same protocol core with the .await confined to a ~10-line refill loop.

Next layers (preview)

  • (2/N) Adopt PacketBuffer in the production NetworkTransport read path, deleting the duplicated TdsReadBuffer scalar/refill logic.
  • Later layers: invert the token/decoder parsers to a sync step() core, add the blocking shell + sync TdsClient surface, rewire mssql-odbc off block_on.

Validation

  • cargo build -p mssql-tds — clean
  • cargo bclippy — clean (-D warnings)
  • cargo bfmt — clean
  • cargo nextest run -p mssql-tds packet_reader — 15/15 pass

Framing (correctness / architecture, not a perf win yet)

This PR is the bottom of a 15-PR sans-I/O stack (#189 through #208) restructuring mssql-tds into one protocol core with a sync shell and an async shell. The whole effort is a correctness/architecture refactor: it deletes the per-row block_on tax and the fast::* sync-fake hack, and lets sync consumers (mssql-odbc, the mssql-py-core sync cursor) and async consumers (the mssql-py-core coroutine cursor) drive the same buffer-driven parser with .await confined to a ~10-line refill loop.

It is not yet a native-beating performance win, and nothing in this stack claims to beat msodbcsql18. The remaining performance debt to burn down next: VarcharMax/PLP streaming (~818x on both the async-before and sync-after variants, pre-existing), the Decimal/DateTime2 conversion gaps, and the per-column SQLGetData conversion/alloc path.

Related work item / issue: Tracked as part of the sans-I/O native stack (#192)

Introduce PacketBuffer, the synchronous I/O-free half of packet reading:
it owns the reassembled payload bytes and serves every scalar/byte read
straight from memory, with the .await confined to the refill edge.

Rewire PacketReader onto PacketBuffer so the buffer math and scalar decode
live in one sans-I/O type; the reader only pulls bytes off the socket on
refill. Public API unchanged.

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

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

77%

🎯 Overall Coverage

91.1%

📦 Project: mssql-tds + mssql-odbc + mssql-py-core
ℹ️ Note: diff coverage is reported, not enforced.


Diff Coverage

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

  • mssql-tds/src/io/packet_buffer.rs (78.7%): Missing lines 64-68,83-85,87-89,91-93,111-113,142,164-165,196-198,201-203
  • mssql-tds/src/io/packet_reader.rs (72.7%): Missing lines 131,133,139,141,155,165

Summary

  • Total: 144 lines
  • Missing: 32 lines
  • Coverage: 77%

mssql-tds/src/io/packet_buffer.rs

  60     /// Callers ensure enough bytes are present (via a refill) before calling; a
  61     /// shortfall here is a protocol/logic error, not a request for more data.
  62     fn take(&mut self, n: usize) -> TdsResult<&[u8]> {
  63         if !self.has(n) {
! 64             return Err(crate::error::Error::ProtocolError(format!(
! 65                 "Buffer underflow: needed {} bytes but only {} available",
! 66                 n,
! 67                 self.available()
! 68             )));
  69         }
  70         let start = self.position;
  71         self.position += n;
  72         if self.position == self.length {

  79     pub(crate) fn take_u8(&mut self) -> TdsResult<u8> {
  80         Ok(self.take(1)?[0])
  81     }
  82 
! 83     pub(crate) fn take_i16_be(&mut self) -> TdsResult<i16> {
! 84         Ok(BigEndian::read_i16(self.take(2)?))
! 85     }
  86 
! 87     pub(crate) fn take_i32_be(&mut self) -> TdsResult<i32> {
! 88         Ok(BigEndian::read_i32(self.take(4)?))
! 89     }
  90 
! 91     pub(crate) fn take_uint40_le(&mut self) -> TdsResult<u64> {
! 92         Ok(LittleEndian::read_uint(self.take(5)?, 5))
! 93     }
  94 
  95     pub(crate) fn take_f32_le(&mut self) -> TdsResult<f32> {
  96         Ok(LittleEndian::read_f32(self.take(4)?))
  97     }

  107     pub(crate) fn take_u16_le(&mut self) -> TdsResult<u16> {
  108         Ok(LittleEndian::read_u16(self.take(2)?))
  109     }
  110 
! 111     pub(crate) fn take_u24_le(&mut self) -> TdsResult<u32> {
! 112         Ok(LittleEndian::read_u24(self.take(3)?))
! 113     }
  114 
  115     pub(crate) fn take_i32_le(&mut self) -> TdsResult<i32> {
  116         Ok(LittleEndian::read_i32(self.take(4)?))
  117     }

  138             if self.position == self.length {
  139                 self.position = 0;
  140                 self.length = 0;
  141             }
! 142         }
  143         to_copy
  144     }
  145 
  146     /// Discards up to `count` readable bytes, returning how many were skipped

  160     /// [`commit_packet`](Self::commit_packet) once the packet has been read.
  161     pub(crate) fn begin_refill(&mut self) -> usize {
  162         let remaining = self.available();
  163         if remaining > 0 {
! 164             self.working_buffer
! 165                 .copy_within(self.position..self.length, 0);
  166         }
  167         self.length = remaining;
  168         self.position = 0;
  169         self.length

  192         BigEndian::read_u16(&self.working_buffer[base + 2..base + 4]) as usize
  193     }
  194 
  195     /// Debug view of the raw bytes read for the packet at `base`.
! 196     pub(crate) fn raw_packet(&self, base: usize, raw_len: usize) -> &[u8] {
! 197         &self.working_buffer[base..base + raw_len]
! 198     }
  199 
  200     /// True once every buffered byte has been consumed.
! 201     pub(crate) fn is_drained(&self) -> bool {
! 202         self.position == self.length
! 203     }
  204 }

mssql-tds/src/io/packet_reader.rs

  127 
  128         // The 8-byte header may arrive split across reads; keep reading until it
  129         // is complete before trusting its declared length.
  130         while received < PacketWriter::PACKET_HEADER_SIZE {
! 131             received += self
  132                 .network_reader_writer
! 133                 .receive(self.buffer.refill_window(base, received))
  134                 .await?;
  135         }
  136 
  137         let packet_size_from_header = self.buffer.packet_header_length(base);

  135         }
  136 
  137         let packet_size_from_header = self.buffer.packet_header_length(base);
  138         while received < packet_size_from_header {
! 139             received += self
  140                 .network_reader_writer
! 141                 .receive(self.buffer.refill_window(base, received))
  142                 .await?;
  143         }
  144 
  145         event!(

  151         use pretty_hex::PrettyHex;
  152         event!(
  153             tracing::Level::DEBUG,
  154             "Packet content: {:?}",
! 155             self.buffer.raw_packet(base, received).hex_dump()
  156         );
  157         Ok(received)
  158     }
  159 }

  161 #[async_trait]
  162 impl TdsPacketReader for PacketReader<'_> {
  163     fn reset_reader(&mut self) {
  164         // Make sure that we have read all the data from the buffer.
! 165         assert!(self.buffer.is_drained());
  166         // No Op after this.
  167     }
  168 
  169     async fn cancel_read_stream(&mut self) -> TdsResult<()> {


🔗 Quick Links

View Azure DevOps Build · Coverage Report

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

Extracts synchronous packet buffering and decoding from PacketReader into a sans-I/O core.

Changes:

  • Adds PacketBuffer for buffering, decoding, copying, and skipping.
  • Rewires PacketReader to handle only asynchronous refills.
  • Registers the internal buffer module.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
mssql-tds/src/io/packet_buffer.rs Implements the sans-I/O packet buffer.
mssql-tds/src/io/packet_reader.rs Delegates buffered reads to PacketBuffer.
mssql-tds/src/io.rs Registers the new internal module.

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

Comment on lines +161 to +165
pub(crate) fn begin_refill(&mut self) -> usize {
let remaining = self.available();
if remaining > 0 {
self.working_buffer
.copy_within(self.position..self.length, 0);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants