Skip to content

TrickleReader hangs on values larger than its buffer, which is what it claims to model #44

Description

@lalinsky

TrickleReader in src/msgpack.zig hangs forever if a test decodes a value larger than the reader's buffer — which is the case its own doc comment says it exists to model:

Models a trickling network stream, where a value can straddle a fill and the reader's buffer can be smaller than the value being decoded.

Its readVec discards the vectors it is handed:

fn readVec(r: *std.Io.Reader, data: [][]u8) std.Io.Reader.Error!usize {
    _ = data;
    const self: *TrickleReader = @fieldParentPtr("reader", r);
    if (self.pos >= self.data.len) return error.EndOfStream;
    if (r.end >= r.buffer.len) return 0;
    ...

It only ever fills the reader's own buffer, one byte at a time. readSliceAll into a destination larger than that buffer fills it, gets 0 back with no progress made, and loops forever. stream and discard return error.EndOfStream unconditionally, so there is no other route.

Reproducer

test "decode a string value larger than the reader buffer" {
    const Msg = struct { s: []const u8 };
    const long = "a" ** 200;

    var encoded: [256]u8 = undefined;
    const bytes = try encodeToBuffer(Msg{ .s = long }, &encoded);

    var buffer: [32]u8 = undefined;
    var trickle = TrickleReader.init(&buffer, bytes);
    const decoded = try decodeLeaky(Msg, std.testing.allocator, &trickle.reader);
    defer std.testing.allocator.free(decoded.s);
    try std.testing.expectEqualStrings(long, decoded.s);
}

Never terminates. I hit this writing the end-to-end test for #43 and dropped it.

Why it matters

The existing tests all stay under the threshold — they decode u64-sized values, or assert error.ReaderBufferTooSmall for the borrowed-key path — so the limitation is invisible until someone writes exactly the test the helper advertises. A hang is also the worst failure mode: no output, no assertion, just a timeout, and the natural suspicion falls on the library rather than the harness.

Worth either making readVec honour its data argument, or narrowing the doc comment to say values must fit in the buffer, so the next person does not spend the time working out where the loop is.

String values are copied out rather than borrowed, so unlike map keys they genuinely have no size limit relative to the reader buffer — that path deserves coverage.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions