Skip to content

Run the accept teardown once, and guard the shutdown union read #184

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
kwsantiago merged 2 commits into from
Aug 12, 2026
Merged

Run the accept teardown once, and guard the shutdown union read #184

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
6e8fbb6
Run the accept teardown once, and guard the shutdown union read
kwsantiago Aug 12, 2026
8b17e90
Take the connection slot next to the teardown flag
kwsantiago Aug 12, 2026
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
101 changes: 92 additions & 9 deletions vendor/httpz.patch
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,58 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
}
}

@@ -782,7 +825,15 @@
@@ -719,7 +762,18 @@
// Linux can wake up multiple epoll fds for a single connection.
return if (err == error.WouldBlock) {} else err;
};
- errdefer posix.close(socket);
+ // Set once the conn owns both the socket and its own memory, i.e.
+ // once the errdefer below can do the whole teardown. Without it
+ // the errdefers unwind LIFO and all three run: disown() destroys
+ // the Conn and closes the socket, then these two destroy it a
+ // second time and close the fd again. Reproduced by injecting a
+ // monitorRead failure: all three cleanups fired for one
+ // connection and the relay aborted. A double
+ // MemoryPool.destroy links a node to itself, so two later
+ // create() calls hand the same Conn to two connections, and the
+ // double close can land on an fd another thread just opened.
+ var conn_owns_teardown = false;
+ errdefer if (!conn_owns_teardown) posix.close(socket);
metrics.connection();

const socket_flags = try posix.fcntl(socket, posix.F.GETFL, 0);
@@ -727,7 +781,7 @@
std.debug.assert(socket_flags & nonblocking == nonblocking);

const conn = try self.conn_mem_pool.create(self.allocator);
- errdefer self.conn_mem_pool.destroy(conn);
+ errdefer if (!conn_owns_teardown) self.conn_mem_pool.destroy(conn);

const ip_address = address.toIOAddress();

@@ -741,13 +795,20 @@
http_conn.stream = .{ .socket = .{ .handle = socket, .address = ip_address } };
http_conn.timeout = now + self.timeout_request;

- self.len += 1;
conn.* = .{
.next = null,
.prev = null,
.protocol = .{ .http = http_conn },
};
self.request_list.insert(conn);
+ // Take the slot and arm the full teardown together, with nothing
+ // fallible between them. The two errdefers above release neither
+ // the slot nor the list entry, so a `try` added in that gap would
+ // leak a worker slot permanently and, past the insert, leave a
+ // freed Conn in the live request_list. These three statements
+ // must stay adjacent.
+ self.len += 1;
+ conn_owns_teardown = true;
errdefer {
conn.close();
self.disown(conn);
@@ -782,7 +843,15 @@

while (c) |conn| {
c = conn.next;
Expand All @@ -138,7 +189,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
switch (http_conn.handover) {
.close, .unknown => {
closed_bool.* = true;
@@ -793,7 +844,7 @@
@@ -793,7 +862,7 @@
// can deliver a .recv for the freed Conn.
loop.remove(conn);
conn.close();
Expand All @@ -147,7 +198,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
},
.disown => {
// When res.disown() was called, we immediately removed
@@ -802,14 +853,14 @@
@@ -802,14 +871,14 @@
// before we get back here.
// https://github.com/karlseguin/http.zig/issues/129#issuecomment-3031411404
closed_bool.* = true;
Expand All @@ -164,7 +215,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
continue;
}

@@ -817,18 +868,70 @@
@@ -817,18 +886,70 @@

const hc: *ws.HandlerConn(WSH) = @ptrCast(@alignCast(ptr));
conn.protocol = .{ .websocket = hc };
Expand Down Expand Up @@ -236,7 +287,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
}

// Entry-point of our thread pool. `thread_buf` is a thread-specific buffer
@@ -885,10 +988,10 @@
@@ -885,10 +1006,10 @@
if (success == false) {
ws_conn.close(.{ .code = 4997, .reason = "wsz" }) catch {};
self.websocket.cleanupConn(hc);
Expand All @@ -249,7 +300,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
} else {
// Release `processing` before re-arming. With EPOLLONESHOT, re-arming
// while still holding `processing` loses a read that arrives in the
@@ -903,20 +1006,77 @@
@@ -903,20 +1024,77 @@
log.debug("({f}) failed to add read event monitor: {}", .{ ws_conn.address, err });
ws_conn.close(.{ .code = 4998, .reason = "wsz" }) catch {};
self.websocket.cleanupConn(hc);
Expand Down Expand Up @@ -329,7 +380,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
self.http_conn_pool.release(http_conn);
self.conn_mem_pool.destroy(conn);
}
@@ -995,7 +1155,8 @@
@@ -995,7 +1173,8 @@
while (conn) |c| {
conn = c.next;
c.close();
Expand All @@ -339,7 +390,39 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
self.http_conn_pool.release(c.protocol.http);
self.conn_mem_pool.destroy(c);
}
@@ -1509,6 +1670,38 @@
@@ -1006,7 +1185,30 @@
var conn = list.head;
while (conn) |c| {
conn = c.next;
- const http_conn = c.protocol.http;
+ // Guard the union read. This is unreachable today: handover_list
+ // has exactly one mutation site (the insert in swapList, always
+ // with a .http conn) plus the processSignal snapshot clear, and
+ // no handover_list.remove exists any more, so no stale
+ // .websocket node can be left as its head. Verified statically
+ // and across 12 shutdown-under-load rounds with upgrades in
+ // flight: zero sightings.
+ //
+ // Guarded anyway because that safety is a non-local invariant
+ // and the failure mode here is severe: reading protocol.http on
+ // a .websocket conn panics in ReleaseSafe, and in ReleaseFast
+ // reinterprets a *ws.HandlerConn as an *HTTPConn, so
+ // posix.close() would close whatever integer lands at that
+ // offset. server.deinit() runs before storage teardown, so a
+ // wild close there can land on the LMDB fd before the final
+ // sync. Skipping leaks one fd in a process that is exiting
+ // anyway, which is strictly better than either outcome.
+ const http_conn = switch (c.protocol) {
+ .http => |hc| hc,
+ .websocket => {
+ log.err("shutdownList found a .websocket conn in an http list; skipping", .{});
+ continue;
+ },
+ };
posix.close(http_conn.stream.socket.handle);
http_conn.deinit(allocator);
}
@@ -1509,6 +1711,38 @@
}

fn release(self: *HTTPConnPool, conn: *HTTPConn) void {
Expand Down Expand Up @@ -378,7 +461,7 @@ diff -ruN upstream/src/worker.zig vendor/src/worker.zig
const conns = self.conns;
self.lock();
const available = self.available;
@@ -1516,7 +1709,6 @@
@@ -1516,7 +1750,6 @@
self.unlock();
conn.deinit(self.allocator);

Expand Down
49 changes: 45 additions & 4 deletions vendor/httpz/src/worker.zig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,15 +762,26 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type {
// Linux can wake up multiple epoll fds for a single connection.
return if (err == error.WouldBlock) {} else err;
};
errdefer posix.close(socket);
// Set once the conn owns both the socket and its own memory, i.e.
// once the errdefer below can do the whole teardown. Without it
// the errdefers unwind LIFO and all three run: disown() destroys
// the Conn and closes the socket, then these two destroy it a
// second time and close the fd again. Reproduced by injecting a
// monitorRead failure: all three cleanups fired for one
// connection and the relay aborted. A double
// MemoryPool.destroy links a node to itself, so two later
// create() calls hand the same Conn to two connections, and the
// double close can land on an fd another thread just opened.
var conn_owns_teardown = false;
errdefer if (!conn_owns_teardown) posix.close(socket);
metrics.connection();

const socket_flags = try posix.fcntl(socket, posix.F.GETFL, 0);
const nonblocking = @as(u32, @bitCast(posix.O{ .NONBLOCK = true }));
std.debug.assert(socket_flags & nonblocking == nonblocking);

const conn = try self.conn_mem_pool.create(self.allocator);
errdefer self.conn_mem_pool.destroy(conn);
errdefer if (!conn_owns_teardown) self.conn_mem_pool.destroy(conn);

const ip_address = address.toIOAddress();

Expand All @@ -784,13 +795,20 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type {
http_conn.stream = .{ .socket = .{ .handle = socket, .address = ip_address } };
http_conn.timeout = now + self.timeout_request;

self.len += 1;
conn.* = .{
.next = null,
.prev = null,
.protocol = .{ .http = http_conn },
};
self.request_list.insert(conn);
// Take the slot and arm the full teardown together, with nothing
// fallible between them. The two errdefers above release neither
// the slot nor the list entry, so a `try` added in that gap would
// leak a worker slot permanently and, past the insert, leave a
// freed Conn in the live request_list. These three statements
// must stay adjacent.
self.len += 1;
conn_owns_teardown = true;
errdefer {
conn.close();
self.disown(conn);
Expand Down Expand Up @@ -1167,7 +1185,30 @@ pub fn NonBlocking(comptime S: type, comptime WSH: type) type {
var conn = list.head;
while (conn) |c| {
conn = c.next;
const http_conn = c.protocol.http;
// Guard the union read. This is unreachable today: handover_list
// has exactly one mutation site (the insert in swapList, always
// with a .http conn) plus the processSignal snapshot clear, and
// no handover_list.remove exists any more, so no stale
// .websocket node can be left as its head. Verified statically
// and across 12 shutdown-under-load rounds with upgrades in
// flight: zero sightings.
//
// Guarded anyway because that safety is a non-local invariant
// and the failure mode here is severe: reading protocol.http on
// a .websocket conn panics in ReleaseSafe, and in ReleaseFast
// reinterprets a *ws.HandlerConn as an *HTTPConn, so
// posix.close() would close whatever integer lands at that
// offset. server.deinit() runs before storage teardown, so a
// wild close there can land on the LMDB fd before the final
// sync. Skipping leaks one fd in a process that is exiting
// anyway, which is strictly better than either outcome.
const http_conn = switch (c.protocol) {
.http => |hc| hc,
.websocket => {
log.err("shutdownList found a .websocket conn in an http list; skipping", .{});
continue;
},
};
posix.close(http_conn.stream.socket.handle);
http_conn.deinit(allocator);
}
Expand Down