perf: avoid stack trace capture in NotEnoughDataError

[arthurschreiber]

Summary

NotEnoughDataError is pure parsing control flow: whenever a parser hits the end of the currently buffered data mid-value, it throws this error, and the caller waits for the next chunk before retrying the read. Because the class extended Error, every one of these throws captured a stack trace that was never used — the error never escapes the parsing internals (all 18 catch sites detect it via instanceof NotEnoughDataError), and neither message nor stack is ever read.

This PR turns it into a plain class that no longer extends Error, with a comment explaining why. Behavior is unchanged: the instanceof checks and the byteCount contract work exactly as before.

Performance

Measured at three levels (Node 24, Linux):

Microbenchmark — cost of one throw new NotEnoughDataError(n) / catch cycle:

	ns per throw/catch
old (extends Error)	~2500
new (plain class)	~780

Parser-level — parsing a stream of 5,000 row tokens (varchar + int columns) delivered in fixed-size chunks via Parser.parseTokens. Smaller chunks mean more throw/retry cycles, since every chunk boundary that lands mid-value triggers one:

chunk size	old rows/sec	new rows/sec	change
64 bytes	~447k	~967k	+115%
512 bytes	~964k	~1,116k	+16%
4096 bytes	~1,119k	~1,167k	+3–4%

End-to-end — repeated SELECTs of 1,000–2,000 row result sets against a real SQL Server with the default 4KB packet size: differences were within run-to-run noise (±3%). At the default packet size a throw only happens roughly once per packet, so the savings are diluted by network and stream overhead; the win grows as packets/chunks shrink relative to row size (e.g. wide rows or values spanning packet boundaries).

Behavior considerations

• The one observable difference is if this error ever leaked out of the parsing internals, callers would receive a non-Error throw. It is fully internal today (audited all catch sites), and the new tests pin down the throw/retry contract.
• Chai's assert.throws(fn, SomeClass) only matches Error subclasses, so the new tests assert via explicit instanceof checks instead.

Test coverage

Previously no test referenced NotEnoughDataError at all, and only a single two-buffer split test exercised the retry path. This PR adds:

• test/unit/token/helpers-test.ts — direct contract tests: read helpers throw NotEnoughDataError with the correct byteCount on incomplete data (fixed-width and length-prefixed reads), and don't throw when enough data is available.
• Byte-at-a-time parsing tests for both the token stream parser and the row parser, which exercise the throw/retry cycle at every single byte boundary.

All new tests pass unchanged against the previous implementation (behavior-pinning). Full unit suite: 407 passing.

🤖 Generated with Claude Code

[claude]

Review: perf: avoid stack trace capture in NotEnoughDataError (#1753)

Overview
Converts NotEnoughDataError from an Error subclass to a plain class, so throw new NotEnoughDataError(n) no longer pays for V8 stack-trace capture on the hot parsing-retry path. Adds direct unit tests for helpers.ts read functions plus byte-at-a-time integration tests for the token-stream and row parsers. Well-scoped, single-purpose change with solid before/after benchmarks in the description.

Correctness

• Walked all ~22 throw sites and every catch site across src/token/*.ts, value-parser.ts, and metadata-parser.ts. Every catch site follows the same shape: catch (err) { if (err instanceof NotEnoughDataError) { retry } throw err; }. None of them touch .message or .stack, so dropping Error inheritance is behavior-preserving as claimed.
• instanceof checks work identically for a plain class, and Chai assert.instanceOf does not care whether the constructor extends Error either. The PR correctly avoids assert.throws(fn, Class) (which chai restricts to Error subclasses) in favor of an explicit try/catch helper.
• Spot-checked the readUsVarChar byte-math in the new test (3-char length prefix, 2 chars of buffered data, expects byteCount = 8): readUInt16LE consumes 2 bytes (offset -> 2), byteLength = 3*2 = 6, offset+byteLength = 8. Matches. Good.
• No no-throw-literal / lint issue: the repo's no-throw-literal is the plain (non-type-aware) ESLint core rule, which only flags literal throws, not class instances, so throw new NotEnoughDataError(...) is fine even though it no longer extends Error.

Performance
The core idea (skip Error's stack capture for exceptions used purely as internal control flow) is a well-established V8 optimization technique, and the benchmark methodology (micro, parser-level at varying chunk sizes, and end-to-end) is thorough and honestly reports where the win shrinks (default 4KB packets: within noise). Good, not overclaiming the real-world impact.

Minor suggestion (non-blocking)
Result right above it uses declare value: T; declare offset: number; specifically to avoid the class-field auto-initializer running before the constructor body (relevant because target is esnext, so TS emits native class fields, which the JS engine initializes to undefined immediately before the constructor runs, then overwrites them). NotEnoughDataError.byteCount is not declared this way, so every new NotEnoughDataError(n) still does one redundant field write before the real assignment. Given the PR's whole point is trimming per-throw overhead, declare byteCount: number; (matching the file's existing convention immediately above, in Result) would close that last bit of gap. Small, but free.

Residual risk (already called out, worth a second look)
The PR description already flags this: if NotEnoughDataError ever did leak past the parsing internals (e.g. a future refactor adds a throw site without a matching catch), it would surface as a non-Error value on a stream error event or promise rejection. Confirmed there is no instanceof Error or generic error-normalization logic elsewhere in src/ that would silently coerce/handle that today, so it would likely surface as a confusing undefined.message-style failure downstream rather than fail loudly. Not a reason to block since the audit is accurate as of this diff, just something to keep in mind if new throw sites are added later without corresponding catch/retry logic.

Test coverage
Good improvement here. Previously NotEnoughDataError had no direct test coverage and only one split-buffer test exercised retries at all. The new byte-at-a-time tests for both Parser.parseTokens and the row parser are a strong way to pin down the throw/retry contract across every possible chunk boundary, and the description confirms they pass unchanged against the pre-PR implementation (true behavior-pinning, not just new-behavior tests).

Summary
Solid, well-tested, low-risk perf change with good benchmarking rigor. Only a small optional nit (declare on byteCount) — nothing blocking.

[claude]

Review

Nice, well-substantiated change. I audited the diff and cross-checked it against the codebase.

Correctness

• Verified all ~19 instanceof NotEnoughDataError catch sites (stream-parser.ts ×12, colmetadata-token-parser.ts ×2, returnvalue-token-parser.ts ×2, nbcrow-token-parser.ts, row-token-parser.ts, metadata-parser.ts) — every one either retries on instanceof NotEnoughDataError or does a bare throw err, so an error that isn't NotEnoughDataError still propagates with its original type/stack intact. Nothing depends on .message, .stack, or instanceof Error for this class specifically. The behavior-preservation claim holds up.
• Spot-checked the new readUsVarChar/readUInt32LE test cases against the actual helper logic (offset math for the length-prefixed and fixed-width reads) — the expected byteCount values line up correctly with what the code computes.
• The declare byteCount change is consistent with the existing Result<T> class right above it, and since the field is unconditionally assigned in the constructor, declare (skip the implicit field definition) vs. a real field declaration is behaviorally identical here.

Code quality

• The comment on NotEnoughDataError explaining why it deliberately doesn't extend Error is exactly the kind of non-obvious-rationale comment worth keeping.
• Small style nit: the class no longer extends anything, so per common TS convention you might consider a JSDoc-style @internal tag or keeping the class un-exported if it's truly 100% internal to the token parsers — but it's already used via import across many parser modules, so keeping it exported as-is is correct; just a thought, not a blocker.

Test coverage

• Good addition — there was previously zero direct coverage of NotEnoughDataError's contract, and the byte-at-a-time parsing tests for both the row and token-stream parsers are a genuinely useful regression guard for the retry path (this is the kind of test that would have caught a bug if the refactor had been wrong).
• One gap: there's no test asserting that a different error type thrown from within a parser (e.g., the throw new Error('Unknown type: ...') in stream-parser.ts) still propagates correctly through one of these catch/retry blocks. Given that's the main behavioral risk called out in the PR description itself, a small test for that would close the loop nicely — though the manual audit above gives me confidence it's fine.

Performance & security

• The benchmark methodology (micro, parser-level at varying chunk sizes, end-to-end) is thorough and the conclusions (bigger win at small chunk/packet sizes, negligible at default 4KB) are exactly what I'd expect from removing Error.captureStackTrace on a hot control-flow path.
• No security concerns — this is purely an internal control-flow class that never crosses a trust boundary or gets serialized/logged.

Overall: solid, low-risk perf win with good test coverage and an honest writeup of the tradeoffs. Approving from a code-review standpoint.

[github-actions]

🎉 This PR is included in version 20.0.5 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀