HTTP client hangs trying to read a response body of 204 No Content

[Darkyenus]

Zig Version

0.16.0-dev.195+ac42eaaad

Steps to Reproduce and Observed Behavior

The HTTP Client will try to read/discard response body even if no body is expected to appear, which will hang forever/until the server closes the connection. Which can take a while with Connection: keep-alive.

const std = @import("std");
test {
    var client: std.http.Client = .{
        .allocator = std.testing.allocator,
    };
    defer client.deinit();

    for ([_]bool{ false, true }) |keep_alive| {// The problem does not happen when keep_alive is false
        for ([_][]const u8{ "http", "https" }) |protocol| {
            const host_url = "httpbun.org";// Arbitrary service to reproduce the problem
            const method: std.http.Method = .DELETE;// Zig expects DELETE response to have a body
            const path = "status/204";// No Content, causes the server to return no Content-Length header

            var response: std.Io.Writer.Allocating = .init(std.testing.allocator);
            defer response.deinit();

            var url_buffer: [512]u8 = undefined;
            const result = try client.fetch(.{
                .response_writer = &response.writer,
                .location = .{ .url = try std.fmt.bufPrint(&url_buffer, "{s}://{s}/{s}", .{ protocol, host_url, path }) },
                .method = method,
                .payload = null,
                .keep_alive = keep_alive,
            });

            try std.testing.expectEqual(.no_content, result.status);
            try std.testing.expectEqual(0, response.written().len);

            std.debug.print("Done {s} {}\n", .{protocol, keep_alive});
        }
    }
}

In this example, when keep_alive is false, everything will work as expected, because while Client will attempt to errorniously discard the body, the server will close the connection and no hang will occur.

But with keep_alive true, the connection will stay open, with no traffic. Because the response code is 204 No Content, the response will not even contain Content-Length: 0 header. The Client will still assume that the response body is everything left in the stream (despite response headers containing HTTP 1.1 and Connection: keep-alive). It will then try to read the body forever and hang here.

Expected Behavior

zig.http.Client (probably function bodyReader in particular) should respect HTTP 1.1 specification on when the response contains the body.

For response messages, whether or not a message-body is included with a message is dependent on both the request method and the response status code. All responses to the HEAD request method MUST NOT include a message-body, even though the presence of entity-header fields might lead one to believe they do. All 1xx (informational), 204 (no content), and 304 (not modified) responses MUST NOT include a message-body. All other responses do include a message-body, although it MAY be of zero length. (RFC-2616, section 4.3)

[Darkyenus]

After further looking into it, it seems that std.http.Method functions requestHasBody and responseHasBody are both flawed and should be deleted.

RFC-2616 section 4.3 says that:

The presence of a message-body in a request is signaled by the
inclusion of a Content-Length or Transfer-Encoding header field in
the request's message-headers. A message-body MUST NOT be included in
a request if the specification of the request method (section 5.1.1)
does not allow sending an entity-body in requests. A server SHOULD
read and forward a message-body on any request; if the request method
does not include defined semantics for an entity-body, then the
message-body SHOULD be ignored when handling the request.

But no request method section actually disallows sending a message-body. Stack Overflow seems to agree that message-body is allowed in any request method (even if the semantics are not always well defined), so requestHasBody should always return true.

And as already mentioned in the issue, responseHasBody cannot be answered purely based on the used method - this depends on the method AND on the response status code. Existence of this method is misleading.

[Darkyenus]

Actually the logic for skipping the response body already exists, it just does nothing - r.response_content_length = head.content_length; just reassigns null and the r.response_content_length is not read in the relevant path - head.content_length is.

Adding head.content_length = 0; just before that line (and changing response to var to allow that change) does fix the issue.

However, it would still be good to delete the HasBody functions - their existence implies that there might be more bugs in the client and/or the server.