Fix Reader.Limited hangs

[whatisaphone]

Fixes #24932, #25093

I'm not 100% sure on the approach, but it does fix those bugs, and the tests pass (and it also fixes my real world project)

It used to be that in Reader.stream, if there is buffered data but you pass a limit of 0, limit.slice would return a buffer with len == 0, then it would fall through the buffer.len check and call vtable.stream (endingStream) even though the stream hadn't ended. That doesn't seem right to me.

I believe that issue masked the two linked bugs in certain conditions, which is why as of 0.15.1 you need a custom Reader or a third intermediary stream to expose them, something like this:

pub fn main() !void {
    var fixed = std.io.Reader.fixed("i'm dying");
    var wyh: std.hash.Wyhash = .init(0);
    var hashed = fixed.hashed(&wyh, &.{});
    var limited = hashed.reader.limited(.limited(2), &.{});
    const r = &limited.interface;

    var out: [2]u8 = undefined;
    try r.readSliceAll(&out);
    std.debug.print("emptied\n", .{});

    try r.readSliceAll(&out);
    std.debug.print("never gets here\n", .{});
}

After the first commit, the linked bugs were straightforward to repro and write tests for in the second commit.

[jeffective]

It might not be appropriate to short-circuit a zero byte stream() in all cases? I think there are some use cases where reading zero bytes is actually valuable, to check if a file descriptor is working or something like that? I'm not sure.

[rohlem]

I think there are some use cases where reading zero bytes is actually valuable, to check if a file descriptor is working or something like that? I'm not sure.

Wouldn't these use cases already break in the general case, with potential composition of multiple readers/writers, buffering, compression etc.?
imo if we want this feature in the reader (or writer) interface, it should/could be a separate vtable entry (maybe named access or checkAvailable?).
Otherwise this could be solved by a specific method/function to call for the concrete type, instead of the general Reader/Writer interfaces.

[whatisaphone]

Right now reading 0 bytes is a no-op, not an error. It's like subtracting 0 from a number. I don't think that should change.

[rohlem]

Ah, my bad, I confused the interface implementation function here (in Reader/Limited.zig) with the user-facing stream function in the interface Reader.zig.
Reader.stream already returns 0 if limit == .nothing was passed, in that case this function isn't called - all good then, sorry for the noise.
(We could document and assert(limit != .nothing); here in the implementation functions, but that's probably deemed unnecessary.)

[andrewrk]

Thanks for this. I've incorporated the first commit into #25512 but I don't like the second commit as-is. Are you sure the second commit is needed in order to fix those issues? If so, I think the bug lies deeper.

[andrewrk]

This should not be logic inside the interface. To enforce this rule, it must be an assertion. There's already an assertion below for this on line 183 but it will be skipped if an error is returned. I suggest instead of this to do the assertion after calling stream but before returning an error. Also, this rule must be mentioned in the VTable documentation for stream.

[whatisaphone]

When I first posted this, ec636c3 was definitely a prerequisite for c95d2b7 to work, but it seems that's changed since then. The tests pass fine on my commit b9f8b6e from #25512, without needing ec636c3.

What's weird is the new tests still pass even if I completely remove my change in stream. However if I remove the corresponding change in discard, the tests hang. So something is out of sync, elsewhere in Io, between the way stream and discard are being called when limit == .nothing. It seems like it should be consistent, but I don't know the grand vision for Io well enough to know which is desired.

At any rate I'm fine closing this in favor of #25512 and we can consider the two linked bugs fixed once that's merged. But do note this line is useless now. I suggest there's some follow-on work of figuring out the stream/discard discrepancy, removing that useless line and (hopefully) its twin, updating docs for stream, and adding the assert you mentioned. I might leave it to someone who knows their way around new Io a little better.

[andrewrk]

Another thing worth noting: not doing the 0 check in stream interface is required in order for the fix in #25162 to resolve the ambiguity between error.StreamTooLong and error.EndOfStream. For instance the Reader could be Reader.fixed and so there isn't any more buffer space to disambiguate. We would then rely on calling stream with a zero-length buffer to be equivalent to checking if it is in an end-of-stream state.

[whatisaphone]

Funny you should say that, because I just tried the test from that commit transplanted to b9f8b6e, and it fails because r.stream() returns error.EndOfStream despite the stream having data. That doesn't seem right does it? Or was that fixed elsewhere?

error: 'Io.Reader.Limited.test.fixed: when streaming 0 bytes, return 0' failed: /home/john/Documents/Dev/Source/zig/lib/std/Io/Reader.zig:1680:5: 0x1090dae in endingStream (std.zig)
    return error.EndOfStream;
    ^
/home/john/Documents/Dev/Source/zig/lib/std/Io/Reader.zig:178:15: 0x1037af2 in stream (std.zig)
    const n = try r.vtable.stream(r, w, limit);
              ^
/home/john/Documents/Dev/Source/zig/lib/std/Io/Reader/Limited.zig:56:15: 0x122b591 in test.fixed: when streaming 0 bytes, return 0 (std.zig)
    const n = try r.stream(&trash.writer, .nothing);