From 1d764c1fdf04829cec5974d82cec901825a80e49 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Fri, 5 Sep 2025 11:26:38 -0700 Subject: [PATCH] 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. --- lib/std/Io/Writer.zig | 64 +++++++++---------------------------------- lib/std/fs/File.zig | 39 ++++++++++++-------------- lib/std/fs/test.zig | 29 -------------------- 3 files changed, 30 insertions(+), 102 deletions(-) diff --git a/lib/std/Io/Writer.zig b/lib/std/Io/Writer.zig index be597ca3e7..1c34a124ae 100644 --- a/lib/std/Io/Writer.zig +++ b/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 { diff --git a/lib/std/fs/File.zig b/lib/std/fs/File.zig index 26b0f0aba5..a5a3955a24 100644 --- a/lib/std/fs/File.zig +++ b/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; } diff --git a/lib/std/fs/test.zig b/lib/std/fs/test.zig index 03d62942a6..65e86e4c2e 100644 --- a/lib/std/fs/test.zig +++ b/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()); -}