test: make TrickleReader honour the destination it is given - #45
Conversation
TrickleReader always wrote its byte into the reader's own buffer and returned 1, which suits only one of the two drivers in std.Io.Reader. fillUnbuffered passes an empty data[0], expects the reader's buffer to grow, and ignores the return value. readSliceShort passes the caller's remaining destination and advances by the returned count. Under the latter the old implementation wrote the byte somewhere the caller never looks while claiming it had been delivered, and once the reader's buffer filled it returned 0 with no progress, so the loop spun forever. Nothing hit it because every existing test kept values inside the buffer, where readSliceShort's opening memcpy satisfies the whole read and the loop never runs. That is below the threshold the type's own doc comment advertises, which says the buffer can be smaller than the value being decoded. Branch on whether a destination was offered, and return 0 for buffer writes as the vtable contract describes. Adds the test this was blocking: a 200 byte string decoded through buffers of 16, 33 and 64 bytes. String values are copied out rather than borrowed, so unlike map keys they have no size limit relative to the reader's buffer. With the old readVec that test does not terminate.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change updates ChangesTrickleReader read-path correction
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Merge Risk: ⚪ Minimal · up to TrickleReader now correctly delivers oversized decoded strings through caller-provided buffers, avoiding the prior zero-progress hang. The added coverage validates this behavior across multiple small buffer sizes, with no remaining merge-readiness risk identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Superseded by #46, which implements |
1 participant
Fixes #44.
TrickleReaderalways wrote its byte into the reader's own buffer and returned 1. That suits only one of the two drivers instd.Io.Reader:fillUnbufferedpasses an emptydata[0], expectsr.bufferto grow, and ignores the return valuereadSliceShortpasses the caller's remaining destination and advances by the returned countUnder the second, the old implementation wrote the byte somewhere the caller never looks while claiming it had been delivered — and once
r.bufferfilled it returned 0 with no progress, so the loop spun forever.Now it branches on whether a destination was offered, and returns 0 for buffer writes as the vtable contract describes ("Implementations may ignore
data, writing directly toReader.buffer... and returning 0").Why it went unnoticed
Every existing test keeps values inside the buffer, where
readSliceShort's opening memcpy satisfies the whole read and the loop never runs. The remaining tests asserterror.ReaderBufferTooSmall, which the library raises before reading. So the helper was only ever exercised below the threshold its own doc comment advertises — that the buffer can be smaller than the value being decoded.The hang is also the worst failure mode: no output, no assertion, just a timeout, with suspicion landing on the library rather than the harness.
The test it was blocking
A 200-byte string decoded through buffers of 16, 33 and 64 bytes. String values are copied out rather than borrowed, so unlike map keys they have no size limit relative to the reader's buffer — a real case with no coverage until now.
Verified both ways: with the old
readVecthe new test does not terminate (killed at 45s); with the fix the suite runs in the usual few hundred milliseconds.Still not implemented
streamanddiscardreturnerror.EndOfStreamunconditionally. Nothing needs them yet, butskipAnyusesdiscardAll, so a future skip test against this reader will fail — loudly and immediately, not by hanging.zig build test: 184/184 pass, up from 183.Summary by CodeRabbit