Skip to content

Commit 2f70f3f

Browse files
committed
fix(switch): no-op manual dispatchInboundStream when the continuous dispatcher owns inbound
A connection's auto inboundDispatcher and a manual conn.dispatchInboundStream both consumed the accept queue, racing for one inbound stream; the loser starved to a timeout (flaky under CPU load). Make the manual dispatch_inbound_stream command an idempotent no-op when dispatcher_running is set — the continuous dispatcher already serves inbound — mirroring the existing no-op at startInboundDispatch. Remove the dead AlreadyDispatching error and document that the on_connected callback must not drive inbound dispatch. Adds a regression test.
1 parent 9c146f3 commit 2f70f3f

1 file changed

Lines changed: 106 additions & 1 deletion

File tree

src/switch.zig

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,11 @@ pub const Switch = struct {
6767
/// way that could block or deadlock.
6868
pub const PeerEventCallback = struct {
6969
ctx: *anyopaque,
70+
/// Inbound dispatch is owned by the Switch (auto_inbound_dispatch /
71+
/// startInboundDispatcher); this callback must NOT start or drive inbound
72+
/// dispatch itself. It fires before the Switch posts its auto-start, so a
73+
/// callback that started dispatch would just be the no-op'd loser of the
74+
/// race with the owning dispatcher.
7075
on_connected: *const fn (ctx: *anyopaque, peer: PeerId, conn: *SwitchConnection, remote_addr: std.Io.net.IpAddress) void,
7176
/// Fired once per CONNECTION unregister, not per peer: two connections to
7277
/// one peer (simultaneous dial) each fire. `conn` identifies WHICH
@@ -117,7 +122,7 @@ pub const Switch = struct {
117122
ConnectionClosed,
118123
SelectedProtocolMismatch,
119124
} || quic.Connection.OpenStreamError || protocols.multistream.Error || std.Io.ConcurrentError;
120-
pub const StartInboundDispatchError = error{ ConnectionClosed, AlreadyDispatching } || std.Io.Cancelable || std.Io.ConcurrentError;
125+
pub const StartInboundDispatchError = error{ConnectionClosed} || std.Io.Cancelable || std.Io.ConcurrentError;
121126
pub const CloseError = error{ConnectionClosed} || quic.Connection.CloseError;
122127

123128
pub const ListenError = error{AddressInvalid} || quic.QuicEndpoint.ListenError;
@@ -398,6 +403,9 @@ pub const Switch = struct {
398403
// Only a genuinely-registered connection reaches here: the append above
399404
// is the last fallible step, so there is no error path between
400405
// registration and firing.
406+
//
407+
// Fires BEFORE the auto-start dispatch below, so the observer learns the
408+
// peer connected before any inbound handler runs.
401409
if (sw.peer_event_callback) |cb| {
402410
cb.on_connected(cb.ctx, managed.peerId(), managed, managed.remoteAddress());
403411
}
@@ -993,6 +1001,15 @@ fn actorMain(actor: *SwitchConnectionActor) std.Io.Cancelable!void {
9931001
) catch |err| cmd.reply.complete(actor.io, err);
9941002
},
9951003
.dispatch_inbound_stream => |cmd| {
1004+
// The continuous dispatcher already owns this connection's accept
1005+
// queue, so a manual single-shot dispatch is satisfied-by-the-running
1006+
// dispatcher: a second consumer would race it for one stream and one
1007+
// would starve. Report success and skip (mirrors the idempotent-no-op
1008+
// at startInboundDispatch).
1009+
if (actor.dispatcher_running) {
1010+
cmd.reply.complete(actor.io, {});
1011+
continue;
1012+
}
9961013
actor.dispatchInboundStream(cmd.opts) catch |err| {
9971014
cmd.reply.complete(actor.io, err);
9981015
if (err == error.Canceled) return error.Canceled;
@@ -1650,6 +1667,94 @@ test "openProtocolStreamMulti negotiates the best protocol the peer supports" {
16501667
server_conn_live = false;
16511668
}
16521669

1670+
test "manual dispatchInboundStream is a no-op when the continuous dispatcher already owns inbound" {
1671+
// With auto_inbound_dispatch at its default (TRUE), accept() auto-starts the
1672+
// continuous dispatcher, which then OWNS the connection's accept queue. A manual
1673+
// dispatchInboundStream on the same connection must NOT spin up a second consumer
1674+
// (the two would race for one stream and one would starve); it must report
1675+
// success via the no-op path. The continuous dispatcher must still service the
1676+
// peer's stream — proving no second consumer stole it.
1677+
const allocator = std.testing.allocator;
1678+
var threaded = std.Io.Threaded.init(allocator, .{});
1679+
defer threaded.deinit();
1680+
const io = threaded.io();
1681+
1682+
var server_key = try identity.KeyPair.generate(.ED25519);
1683+
defer server_key.deinit();
1684+
var client_key = try identity.KeyPair.generate(.ED25519);
1685+
defer client_key.deinit();
1686+
1687+
const server_endpoint = try quic.QuicEndpoint.initWithIdentity(allocator, io, &server_key, .{});
1688+
defer server_endpoint.deinit();
1689+
const client_endpoint = try quic.QuicEndpoint.initWithIdentity(allocator, io, &client_key, .{});
1690+
defer client_endpoint.deinit();
1691+
1692+
const server = try Switch.init(allocator, io, server_endpoint);
1693+
defer server.deinit();
1694+
const client = try Switch.init(allocator, io, client_endpoint);
1695+
defer client.deinit();
1696+
// Leave server.auto_inbound_dispatch at its default TRUE: accept() auto-starts
1697+
// the continuous dispatcher that owns inbound here.
1698+
1699+
// Echo one byte and record that the auto-dispatcher's handler ran.
1700+
const EchoHandler = struct {
1701+
queue: *std.Io.Queue(u8),
1702+
fn run(self: *@This(), handler_io: std.Io, stream: *quic.Stream) anyerror!void {
1703+
var buf: [1]u8 = undefined;
1704+
try stream.readAll(handler_io, &buf, .{});
1705+
try stream.writeAll(handler_io, &buf, .{});
1706+
try self.queue.putOne(handler_io, buf[0]);
1707+
}
1708+
};
1709+
var queue_buffer: [1]u8 = undefined;
1710+
var queue = std.Io.Queue(u8).init(&queue_buffer);
1711+
var handler = EchoHandler{ .queue = &queue };
1712+
try server.addProtocolService(
1713+
"/test/noop/1.0.0",
1714+
protocols.streamHandlerService(EchoHandler, EchoHandler.run, &handler),
1715+
);
1716+
1717+
var listen_addr = try Multiaddr.fromString(allocator, "/ip4/127.0.0.1/udp/0/quic-v1");
1718+
defer listen_addr.deinit(allocator);
1719+
try server.listen(listen_addr);
1720+
var client_listen_addr = try Multiaddr.fromString(allocator, "/ip4/127.0.0.1/udp/0/quic-v1");
1721+
defer client_listen_addr.deinit(allocator);
1722+
try client.listen(client_listen_addr);
1723+
1724+
var addrs = try server.listenMultiaddrs(allocator);
1725+
defer {
1726+
for (addrs.items) |addr| allocator.free(addr);
1727+
addrs.deinit(allocator);
1728+
}
1729+
var dial_addr = try Multiaddr.fromString(allocator, addrs.items[0]);
1730+
defer dial_addr.deinit(allocator);
1731+
1732+
const client_conn = try client.dial(dial_addr, .{});
1733+
defer client_conn.deinit();
1734+
// accept() runs auto-start: the continuous dispatcher now owns server inbound.
1735+
const server_conn = try server.accept();
1736+
defer server_conn.deinit();
1737+
1738+
// The no-op path returns immediately (it never calls acceptStream), so a short
1739+
// timeout still proves "success, not error": a real second consumer would block
1740+
// here until the timeout. Assert void, not an error.
1741+
try server_conn.dispatchInboundStream(.{
1742+
.accept_timeout = .{ .duration = .{ .raw = .fromNanoseconds(100 * std.time.ns_per_ms), .clock = .awake } },
1743+
});
1744+
1745+
// The continuous auto-dispatcher still services the client's stream: it echoes
1746+
// the byte and records that its handler ran — so no second consumer stole it.
1747+
const stream = try client_conn.openProtocolStream("/test/noop/1.0.0", .{});
1748+
defer stream.deinit();
1749+
defer closeStreamForCleanup(io, stream);
1750+
var out = [_]u8{0x7E};
1751+
try stream.writeAll(io, &out, .{});
1752+
var in: [1]u8 = undefined;
1753+
try stream.readAll(io, &in, .{});
1754+
try std.testing.expectEqual(@as(u8, 0x7E), in[0]);
1755+
try std.testing.expectEqual(@as(u8, 0x7E), try queue.getOne(io));
1756+
}
1757+
16531758
test "a stalled outbound negotiation does not block the connection's command lane" {
16541759
const allocator = std.testing.allocator;
16551760
var threaded = std.Io.Threaded.init(allocator, .{});

0 commit comments

Comments
 (0)