Skip to content

Fix connectUnix and allow passing connections to fetch() #23948

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.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BrainBlasted
Copy link

connectUnix() was broken by the changes to how we manage the connection pool that were made when length tracking was removed from std.DoublyLinkedList. If a developer attempted to use it currently, it would fail to compile due to it relying on incorrect types.

In addition, it was previously impossible to use the client.fetch() API to work with unix sockets. If a developer wanted to make a request on a socket, they were required to use the low-level request API via client.open(). This lead to code like the following:

fn createContainerRequest(
    allocator: Allocator,
    client: *http.Client,
    socket: []const u8,
) ![]u8 {
    var arena = ArenaAllocator.init(allocator);
    defer arena.deinit();

    const aa = arena.allocator();

    const uri = try std.Uri.parse("http://localhost/v1.49/containers/create");

    // SNIPPED:  const payload = ...
    const json = try std.json.stringifyAlloc(aa, payload, .{});

    const conn = try client.connectUnix(socket);

    var header_buffer = std.mem.zeroes([2048]u8);
    var req = try client.open(.POST, uri, .{
        .connection = conn,
        .server_header_buffer = &header_buffer,
        .headers = .{ .content_type = .{ .override = "application/json" } },
    });
    defer req.deinit();

    req.transfer_encoding = .{ .content_length = json.len };

    try req.send();
    try req.writeAll(json);
    try req.finish();
    try req.wait();

    var reader = req.reader();
    const res = try reader.readAllAlloc(aa, 4096);

    if (req.response.status.class() != .success) {
        std.debug.print("Error: {s}\n", .{res});
        return error.CreationFailed;
    }

    const container_res = try std.json.parseFromSliceLeaky(struct { Id: []u8 }, aa, res, .{
        .ignore_unknown_fields = true,
    });
    return try allocator.dupe(u8, container_res.Id);
}

If we allow developers to use the fetch API, the function body can be greatly simplified:

    var arena = ArenaAllocator.init(allocator);
    defer arena.deinit();

    const aa = arena.allocator();

    // SNIPPED: const payload = ...
    const json = try std.json.stringifyAlloc(aa, payload, .{});

    const conn = try client.connectUnix(socket);
    var data = std.ArrayList(u8).init(aa);

    const res = try client.fetch(.{
        .method = .POST,
        .connection = conn,
        .location = .{ .url = "http://localhost/v1.49/containers/create" },
        .headers = .{ .content_type = .{ .override = "application/json" } },
        .payload = json,
        .response_storage = .{
            .dynamic = &data,
        },
    });

    if (res.status.class() != .success) {
        std.debug.print("Error: {s}\n", .{data.items});
        return error.CreationFailed;
    }

    const container_res = try std.json.parseFromSliceLeaky(struct { Id: []u8 }, aa, data.items, .{
        .ignore_unknown_fields = true,
    });
    return try allocator.dupe(u8, container_res.Id);

`connectUnix()` was broken by the changes to how we manage the
connection pool that were made when length tracking was removed
from `std.DoublyLinkedList`. If a developer attempted to use it
currently, it would fail to compile due to it relying on incorrect
types.

This commit fixes all compilation errors in this function, bringing
it in line with `connectTcp()`.
This allows developers to easily use fetch for unix sockets,
instead of having to use the low-level http.Client API.
@andrewrk andrewrk self-requested a review May 21, 2025 02:15
@andrewrk
Copy link
Member

conflicts with work in progress

@BrainBlasted
Copy link
Author

Is it just the connectUnix part that conflicts? Or do the changes to fetch conflict as well?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants