Skip to content

http.BodyWriter: handle EOF in chunkedSendFile, simplify #24864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andrewrk merged 2 commits into from
Aug 16, 2025
Merged

http.BodyWriter: handle EOF in chunkedSendFile, simplify #24864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ce4e8a9
std-docs: improve error message on write failure
ifreund Aug 15, 2025
0cfd07b
http.BodyWriter: handle EOF in chunkedSendFile, simplify
ifreund Aug 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions lib/compiler/std-docs.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,19 @@ fn accept(context: *Context, connection: std.net.Server.Connection) void {
return;
},
};
serveRequest(&request, context) catch |err| {
std.log.err("unable to serve {s}: {s}", .{ request.head.target, @errorName(err) });
return;
serveRequest(&request, context) catch |err| switch (err) {
error.WriteFailed => {
if (conn_writer.err) |e| {
std.log.err("unable to serve {s}: {s}", .{ request.head.target, @errorName(e) });
} else {
std.log.err("unable to serve {s}: {s}", .{ request.head.target, @errorName(err) });
}
return;
},
else => {
std.log.err("unable to serve {s}: {s}", .{ request.head.target, @errorName(err) });
return;
},
};
}
}
Expand Down
163 changes: 52 additions & 111 deletions lib/std/http.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -758,22 +758,14 @@ pub const BodyWriter = struct {
/// As a debugging utility, counts down to zero as bytes are written.
content_length: u64,
/// Each chunk is wrapped in a header and trailer.
chunked: Chunked,
/// This length is the the number of bytes to be written before the
/// next header. This includes +2 for the `\r\n` trailer and is zero
/// for the beginning of the stream.
chunk_len: usize,
/// Cleanly finished stream; connection can be reused.
end,

pub const Chunked = union(enum) {
/// Index to the start of the hex-encoded chunk length in the chunk
/// header within the buffer of `BodyWriter.http_protocol_output`.
/// Buffered chunk data starts here plus length of `chunk_header_template`.
offset: usize,
/// We are in the middle of a chunk and this is how many bytes are
/// left until the next header. This includes +2 for "\r"\n", and
/// is zero for the beginning of the stream.
chunk_len: usize,

pub const init: Chunked = .{ .chunk_len = 0 };
};
pub const init_chunked: State = .{ .chunk_len = 0 };
};

pub fn isEliding(w: *const BodyWriter) bool {
Expand All @@ -784,21 +776,7 @@ pub const BodyWriter = struct {
pub fn flush(w: *BodyWriter) Error!void {
const out = w.http_protocol_output;
switch (w.state) {
.end, .none, .content_length => return out.flush(),
.chunked => |*chunked| switch (chunked.*) {
.offset => |offset| {
const chunk_len = out.end - offset - chunk_header_template.len;
if (chunk_len > 0) {
writeHex(out.buffer[offset..][0..chunk_len_digits], chunk_len);
chunked.* = .{ .chunk_len = 2 };
} else {
out.end = offset;
chunked.* = .{ .chunk_len = 0 };
}
try out.flush();
},
.chunk_len => return out.flush(),
},
Comment on lines -787 to -801

@andrewrk andrewrk Aug 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to give you some background for why this was done: the idea was that when flushing a chunked http transfer request, it should finish the chunk and send the end-of-chunk trailer and start a new chunk. Looking at it with fresh eyes, I'm not sure how valuable that is, although I can think of some reasons for it.

anyway I think the simplification that you brought is nice, so let's run with that.

@ifreund ifreund Aug 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this offset code in flush was actually unreachable before my commit.

I don't think it's possible to implement what you describe either. We can't finish the chunk early without writing the full amount of data specified in the chunk header, and we don't have access to data that the user of the API started but did not finish writing here in flush().

.end, .none, .content_length, .chunk_len => return out.flush(),
}
}

Expand Down Expand Up @@ -841,7 +819,7 @@ pub const BodyWriter = struct {
w.state = .end;
},
.none => {},
.chunked => return endChunkedUnflushed(w, .{}),
.chunk_len => return endChunkedUnflushed(w, .{}),
}
}

Expand Down Expand Up @@ -877,24 +855,16 @@ pub const BodyWriter = struct {
/// * `endUnflushed`
/// * `end`
pub fn endChunkedUnflushed(w: *BodyWriter, options: EndChunkedOptions) Error!void {
const chunked = &w.state.chunked;
if (w.isEliding()) {
w.state = .end;
return;
}
const bw = w.http_protocol_output;
switch (chunked.*) {
.offset => |offset| {
const chunk_len = bw.end - offset - chunk_header_template.len;
writeHex(bw.buffer[offset..][0..chunk_len_digits], chunk_len);
try bw.writeAll("\r\n");
},
.chunk_len => |chunk_len| switch (chunk_len) {
0 => {},
1 => try bw.writeByte('\n'),
2 => try bw.writeAll("\r\n"),
else => unreachable, // An earlier write call indicated more data would follow.
},
switch (w.state.chunk_len) {
0 => {},
1 => try bw.writeByte('\n'),
2 => try bw.writeAll("\r\n"),
else => unreachable, // An earlier write call indicated more data would follow.
}
try bw.writeAll("0\r\n");
for (options.trailers) |trailer| {
Expand Down Expand Up @@ -991,44 +961,32 @@ pub const BodyWriter = struct {
return error.Unimplemented;
};
const out = bw.http_protocol_output;
const chunked = &bw.state.chunked;
state: switch (chunked.*) {
.offset => |off| {
// TODO: is it better perf to read small files into the buffer?
const buffered_len = out.end - off - chunk_header_template.len;
const chunk_len = data_len + buffered_len;
writeHex(out.buffer[off..][0..chunk_len_digits], chunk_len);
switch (bw.state.chunk_len) {
0 => {
const header_buf = try out.writableArray(chunk_header_template.len);
@memcpy(header_buf, chunk_header_template);
writeHex(header_buf[0..chunk_len_digits], data_len);
const n = try out.sendFileHeader(w.buffered(), file_reader, limit);
chunked.* = .{ .chunk_len = data_len + 2 - n };
return w.consume(n);
bw.state.chunk_len = data_len + 2 - n;
const ret = w.consume(n);
return ret;
},
.chunk_len => |chunk_len| l: switch (chunk_len) {
0 => {
const off = out.end;
const header_buf = try out.writableArray(chunk_header_template.len);
@memcpy(header_buf, chunk_header_template);
chunked.* = .{ .offset = off };
continue :state .{ .offset = off };
},
1 => {
try out.writeByte('\n');
chunked.chunk_len = 0;
continue :l 0;
},
2 => {
try out.writeByte('\r');
chunked.chunk_len = 1;
continue :l 1;
},
else => {
const chunk_limit: std.Io.Limit = .limited(chunk_len - 2);
const n = if (chunk_limit.subtract(w.buffered().len)) |sendfile_limit|
try out.sendFileHeader(w.buffered(), file_reader, sendfile_limit.min(limit))
else
try out.write(chunk_limit.slice(w.buffered()));
chunked.chunk_len = chunk_len - n;
return w.consume(n);
},
1 => unreachable,

@andrewrk andrewrk Aug 16, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really unreachable? it looks like it could be reached from line 970 setting chunk_len to 1, or line 987 setting chunk_len to 1.

@ifreund ifreund Aug 17, 2025 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is unreachable. It is in fact an assertion that we do not violate the protocol by e.g. writing only \n rather than \r\n or writing more data than specified in the chunk header. I do see a way to make this code a bit more clear though.

@ifreund ifreund Aug 17, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I opened #24884 to further improve the clarity of this code and document the assertions better.

2 => {
try out.writeAll("\r\n");
bw.state.chunk_len = 0;
assert(file_reader.atEnd());
return error.EndOfStream;
},
else => {
const chunk_limit: std.Io.Limit = .limited(bw.state.chunk_len - 2);
const n = if (chunk_limit.subtract(w.buffered().len)) |sendfile_limit|
try out.sendFileHeader(w.buffered(), file_reader, sendfile_limit.min(limit))
else
try out.write(chunk_limit.slice(w.buffered()));
bw.state.chunk_len -= n;
const ret = w.consume(n);
return ret;
},
}
}
Expand All @@ -1038,42 +996,25 @@ pub const BodyWriter = struct {
assert(!bw.isEliding());
const out = bw.http_protocol_output;
const data_len = w.end + Writer.countSplat(data, splat);
const chunked = &bw.state.chunked;
state: switch (chunked.*) {
.offset => |offset| {
if (out.unusedCapacityLen() >= data_len) {
return w.consume(out.writeSplatHeader(w.buffered(), data, splat) catch unreachable);
}
const buffered_len = out.end - offset - chunk_header_template.len;
const chunk_len = data_len + buffered_len;
writeHex(out.buffer[offset..][0..chunk_len_digits], chunk_len);
l: switch (bw.state.chunk_len) {
0 => {
const header_buf = try out.writableArray(chunk_header_template.len);
@memcpy(header_buf, chunk_header_template);
writeHex(header_buf[0..chunk_len_digits], data_len);
const n = try out.writeSplatHeader(w.buffered(), data, splat);
chunked.* = .{ .chunk_len = data_len + 2 - n };
bw.state.chunk_len = data_len + 2 - n;
return w.consume(n);
},
.chunk_len => |chunk_len| l: switch (chunk_len) {
0 => {
const offset = out.end;
const header_buf = try out.writableArray(chunk_header_template.len);
@memcpy(header_buf, chunk_header_template);
chunked.* = .{ .offset = offset };
continue :state .{ .offset = offset };
},
1 => {
try out.writeByte('\n');
chunked.chunk_len = 0;
continue :l 0;
},
2 => {
try out.writeByte('\r');
chunked.chunk_len = 1;
continue :l 1;
},
else => {
const n = try out.writeSplatHeaderLimit(w.buffered(), data, splat, .limited(chunk_len - 2));
chunked.chunk_len = chunk_len - n;
return w.consume(n);
},
1 => unreachable,
2 => {
try out.writeAll("\r\n");
bw.state.chunk_len = 0;
continue :l 0;
},
else => {
const n = try out.writeSplatHeaderLimit(w.buffered(), data, splat, .limited(bw.state.chunk_len - 2));
bw.state.chunk_len -= n;
return w.consume(n);
},
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/std/http/Client.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,7 +907,7 @@ pub const Request = struct {
return switch (r.transfer_encoding) {
.chunked => .{
.http_protocol_output = http_protocol_output,
.state = .{ .chunked = .init },
.state = .init_chunked,
.writer = .{
.buffer = buffer,
.vtable = &.{
Expand Down
6 changes: 3 additions & 3 deletions lib/std/http/Server.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -448,11 +448,11 @@ pub const Request = struct {
try out.writeAll("\r\n");
const elide_body = request.head.method == .HEAD;
const state: http.BodyWriter.State = if (o.transfer_encoding) |te| switch (te) {
.chunked => .{ .chunked = .init },
.chunked => .init_chunked,
.none => .none,
} else if (options.content_length) |len| .{
.content_length = len,
} else .{ .chunked = .init };
} else .init_chunked;

return if (elide_body) .{
.http_protocol_output = request.server.out,
Expand All @@ -478,7 +478,7 @@ pub const Request = struct {
.drain = http.BodyWriter.contentLengthDrain,
.sendFile = http.BodyWriter.contentLengthSendFile,
},
.chunked => &.{
.chunk_len => &.{
.drain = http.BodyWriter.chunkedDrain,
.sendFile = http.BodyWriter.chunkedSendFile,
},
Expand Down
Loading