Commit 1d764c1fdf

Andrew Kelley <andrew@ziglang.org>
2025-09-05 20:26:38
Revert "Merge pull request #24905 from gooncreeper/file-reader-buffered"
This reverts commit ac42eaaadd0650ffc281f9a1ed1a642fde8984b7, reversing changes made to 9fa2394f8c00d060931d69fb6f342f7f2e3d826e. I would like a chance to review this, please. I already spotted some issues.
1 parent ac42eaa
Changed files (3)
lib/std/fs/File.zig
@@ -1154,7 +1154,6 @@ pub const Reader = struct {
         };
     }
 
-    /// If `error.EndOfStream` has been hit, this cannot fail.
     pub fn getSize(r: *Reader) SizeError!u64 {
         return r.size orelse {
             if (r.size_err) |err| return err;
@@ -1441,7 +1440,7 @@ pub const Reader = struct {
         }
     }
 
-    fn readPositional(r: *Reader, dest: []u8) std.Io.Reader.Error!usize {
+    pub fn readPositional(r: *Reader, dest: []u8) std.Io.Reader.Error!usize {
         const n = r.file.pread(dest, r.pos) catch |err| switch (err) {
             error.Unseekable => {
                 r.mode = r.mode.toStreaming();
@@ -1468,7 +1467,7 @@ pub const Reader = struct {
         return n;
     }
 
-    fn readStreaming(r: *Reader, dest: []u8) std.Io.Reader.Error!usize {
+    pub fn readStreaming(r: *Reader, dest: []u8) std.Io.Reader.Error!usize {
         const n = r.file.read(dest) catch |err| {
             r.err = err;
             return error.ReadFailed;
@@ -1481,6 +1480,14 @@ pub const Reader = struct {
         return n;
     }
 
+    pub fn read(r: *Reader, dest: []u8) std.Io.Reader.Error!usize {
+        switch (r.mode) {
+            .positional, .positional_reading => return readPositional(r, dest),
+            .streaming, .streaming_reading => return readStreaming(r, dest),
+            .failure => return error.ReadFailed,
+        }
+    }
+
     pub fn atEnd(r: *Reader) bool {
         // Even if stat fails, size is set when end is encountered.
         const size = r.size orelse return false;
@@ -1796,15 +1803,9 @@ pub const Writer = struct {
                 file_reader.size = file_reader.pos;
                 return error.EndOfStream;
             }
-            const n = io_w.consume(@intCast(sbytes));
-            if (n <= file_reader.interface.bufferedLen()) {
-                file_reader.interface.toss(n);
-            } else {
-                const direct_n = n - file_reader.interface.bufferedLen();
-                file_reader.interface.tossBuffered();
-                file_reader.seekBy(@intCast(direct_n)) catch return error.ReadFailed;
-            }
-            return n;
+            const consumed = io_w.consume(@intCast(sbytes));
+            file_reader.seekTo(file_reader.pos + consumed) catch return error.ReadFailed;
+            return consumed;
         }
 
         if (native_os.isDarwin() and w.mode == .streaming) sf: {
@@ -1863,15 +1864,9 @@ pub const Writer = struct {
                 file_reader.size = file_reader.pos;
                 return error.EndOfStream;
             }
-            const n = io_w.consume(@bitCast(len));
-            if (n <= file_reader.interface.bufferedLen()) {
-                file_reader.interface.toss(n);
-            } else {
-                const direct_n = n - file_reader.interface.bufferedLen();
-                file_reader.interface.tossBuffered();
-                file_reader.seekBy(@intCast(direct_n)) catch return error.ReadFailed;
-            }
-            return n;
+            const consumed = io_w.consume(@bitCast(len));
+            file_reader.seekTo(file_reader.pos + consumed) catch return error.ReadFailed;
+            return consumed;
         }
 
         if (native_os == .linux and w.mode == .streaming) sf: {
@@ -2003,7 +1998,7 @@ pub const Writer = struct {
         reader_buffered: []const u8,
     ) std.Io.Writer.FileError!usize {
         const n = try drain(io_w, &.{reader_buffered}, 1);
-        file_reader.interface.toss(n);
+        file_reader.seekTo(file_reader.pos + n) catch return error.ReadFailed;
         return n;
     }
 
lib/std/fs/test.zig
@@ -2180,32 +2180,3 @@ test "seekTo flushes buffered data" {
     try file_reader.interface.readSliceAll(&buf);
     try std.testing.expectEqualStrings(contents, &buf);
 }
-
-test "File.Writer sendfile with buffered contents" {
-    var tmp_dir = testing.tmpDir(.{});
-    defer tmp_dir.cleanup();
-
-    try tmp_dir.dir.writeFile(.{ .sub_path = "a", .data = "bcd" });
-    const in = try tmp_dir.dir.openFile("a", .{});
-    defer in.close();
-    const out = try tmp_dir.dir.createFile("b", .{});
-    defer out.close();
-
-    var in_buf: [2]u8 = undefined;
-    var in_r = in.reader(&in_buf);
-    _ = try in_r.getSize(); // Catch seeks past end by populating size
-    try in_r.interface.fill(2);
-
-    var out_buf: [1]u8 = undefined;
-    var out_w = out.writerStreaming(&out_buf);
-    try out_w.interface.writeByte('a');
-    try testing.expectEqual(3, try out_w.interface.sendFileAll(&in_r, .unlimited));
-    try out_w.interface.flush();
-
-    var check = try tmp_dir.dir.openFile("b", .{});
-    defer check.close();
-    var check_buf: [4]u8 = undefined;
-    var check_r = check.reader(&check_buf);
-    try testing.expectEqualStrings("abcd", try check_r.interface.take(4));
-    try testing.expectError(error.EndOfStream, check_r.interface.takeByte());
-}
lib/std/Io/Writer.zig
@@ -921,8 +921,7 @@ pub fn sendFileHeader(
 /// Asserts nonzero buffer capacity.
 pub fn sendFileReading(w: *Writer, file_reader: *File.Reader, limit: Limit) FileReadingError!usize {
     const dest = limit.slice(try w.writableSliceGreedy(1));
-    const n = try file_reader.interface.readSliceShort(dest);
-    if (n == 0) return error.EndOfStream;
+    const n = try file_reader.read(dest);
     w.advance(n);
     return n;
 }
@@ -935,24 +934,17 @@ pub fn sendFileReading(w: *Writer, file_reader: *File.Reader, limit: Limit) File
 ///
 /// Asserts nonzero buffer capacity.
 pub fn sendFileAll(w: *Writer, file_reader: *File.Reader, limit: Limit) FileAllError!usize {
-    // The fallback case uses `stream`. For `File.Reader`, this requires a minumum buffer size of
-    // one since it uses `writableSliceGreedy(1)`. Asserting this here ensures that this will be
-    // hit even when the fallback is not needed.
+    // The fallback sendFileReadingAll() path asserts non-zero buffer capacity.
+    // Explicitly assert it here as well to ensure the assert is hit even if
+    // the fallback path is not taken.
     assert(w.buffer.len > 0);
-
     var remaining = @intFromEnum(limit);
     while (remaining > 0) {
         const n = sendFile(w, file_reader, .limited(remaining)) catch |err| switch (err) {
             error.EndOfStream => break,
             error.Unimplemented => {
                 file_reader.mode = file_reader.mode.toReading();
-                while (remaining > 0) {
-                    remaining -= file_reader.interface.stream(w, .limited(remaining)) catch |e| switch (e) {
-                        error.EndOfStream => break,
-                        error.ReadFailed => return error.ReadFailed,
-                        error.WriteFailed => return error.WriteFailed,
-                    };
-                }
+                remaining -= try w.sendFileReadingAll(file_reader, .limited(remaining));
                 break;
             },
             else => |e| return e,
@@ -2284,12 +2276,6 @@ pub const Discarding = struct {
         const d: *Discarding = @alignCast(@fieldParentPtr("writer", w));
         d.count += w.end;
         w.end = 0;
-        const buffered_n = limit.minInt64(file_reader.interface.bufferedLen());
-        if (buffered_n != 0) {
-            file_reader.interface.toss(buffered_n);
-            d.count += buffered_n;
-            return buffered_n;
-        }
         if (limit == .nothing) return 0;
         if (file_reader.getSize()) |size| {
             const n = limit.minInt64(size - file_reader.pos);
@@ -2781,9 +2767,7 @@ pub const Allocating = struct {
         if (additional == 0) return error.EndOfStream;
         a.ensureUnusedCapacity(limit.minInt64(additional)) catch return error.WriteFailed;
         const dest = limit.slice(a.writer.buffer[a.writer.end..]);
-        const n = try file_reader.interface.readSliceShort(dest);
-        // If it was a short read, then EOF has been reached and `file_reader.size`
-        // has been set and the EOF case will be hit on subsequent calls.
+        const n = try file_reader.read(dest);
         a.writer.end += n;
         return n;
     }
@@ -2834,18 +2818,18 @@ test "discarding sendFile" {
 
     const file = try tmp_dir.dir.createFile("input.txt", .{ .read = true });
     defer file.close();
-    var r_buffer: [2]u8 = undefined;
+    var r_buffer: [256]u8 = undefined;
     var file_writer: std.fs.File.Writer = .init(file, &r_buffer);
-    try file_writer.interface.writeAll("abcd");
+    try file_writer.interface.writeByte('h');
     try file_writer.interface.flush();
 
     var file_reader = file_writer.moveToReader();
     try file_reader.seekTo(0);
-    try file_reader.interface.fill(2);
 
     var w_buffer: [256]u8 = undefined;
     var discarding: Writer.Discarding = .init(&w_buffer);
-    try testing.expectEqual(4, discarding.writer.sendFileAll(&file_reader, .unlimited));
+
+    _ = try file_reader.interface.streamRemaining(&discarding.writer);
 }
 
 test "allocating sendFile" {
@@ -2854,40 +2838,18 @@ test "allocating sendFile" {
 
     const file = try tmp_dir.dir.createFile("input.txt", .{ .read = true });
     defer file.close();
-    var r_buffer: [2]u8 = undefined;
+    var r_buffer: [256]u8 = undefined;
     var file_writer: std.fs.File.Writer = .init(file, &r_buffer);
-    try file_writer.interface.writeAll("abcd");
+    try file_writer.interface.writeByte('h');
     try file_writer.interface.flush();
 
     var file_reader = file_writer.moveToReader();
     try file_reader.seekTo(0);
-    try file_reader.interface.fill(2);
 
     var allocating: Writer.Allocating = .init(testing.allocator);
     defer allocating.deinit();
-    try allocating.ensureUnusedCapacity(1);
-    try testing.expectEqual(4, allocating.writer.sendFileAll(&file_reader, .unlimited));
-    try testing.expectEqualStrings("abcd", allocating.writer.buffered());
-}
 
-test sendFileReading {
-    var tmp_dir = testing.tmpDir(.{});
-    defer tmp_dir.cleanup();
-
-    const file = try tmp_dir.dir.createFile("input.txt", .{ .read = true });
-    defer file.close();
-    var r_buffer: [2]u8 = undefined;
-    var file_writer: std.fs.File.Writer = .init(file, &r_buffer);
-    try file_writer.interface.writeAll("abcd");
-    try file_writer.interface.flush();
-
-    var file_reader = file_writer.moveToReader();
-    try file_reader.seekTo(0);
-    try file_reader.interface.fill(2);
-
-    var w_buffer: [1]u8 = undefined;
-    var discarding: Writer.Discarding = .init(&w_buffer);
-    try testing.expectEqual(4, discarding.writer.sendFileReadingAll(&file_reader, .unlimited));
+    _ = try file_reader.interface.streamRemaining(&allocating.writer);
 }
 
 test writeStruct {