Skip to content

Commit d5c1d56

Browse files
committed
refactor(driver/ipc): replace reqID multiplexer with channel-based serial semaphore
Prior design kept a map[uint64]chan of in-flight requests, which let concurrent goroutines race and interleave responses. Replace with a single waitSem (capacity-1 channel) that serialises request/reply pairs while keeping push frames (cmdRecv, cmdAccept, cmdCloseOK) routed through per-connection channels concurrently. Key changes: - Drop 8-byte reqID from envelope header (ipcEnvelopeHeaderSize: 9→1) - ipcClient.pending is now a chan *pendingResponse (capacity 16) instead of a map; readLoop writes known response cmds directly, sendAndWait reads - doneCh close wakes goroutines blocked on waitSem to prevent deadlock when daemon disconnects while many goroutines are queued - Disconnect() becomes fire-and-forget (send, not sendAndWait) since the daemon never replies to cmdClose in a way sendAndWait can safely demux - cmdCancel removed (no longer needed; timeout races are handled by timer beating doneCh before semaphore is acquired)
1 parent b1ecb8a commit d5c1d56

9 files changed

Lines changed: 164 additions & 193 deletions

pkg/daemon/zz_ipc_bind_accept_test.go

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -131,32 +131,31 @@ func TestHandleBindPortAlreadyBoundReturnsError(t *testing.T) {
131131
assertErrorReply(t, reply, "already bound")
132132
}
133133

134-
// --- handleSend: missing connID (len < 4) emits CmdError ---
134+
// --- handleSend: fire-and-forget — no reply on error ---
135135

136136
func TestHandleSendShortPayloadEmitsError(t *testing.T) {
137137
t.Parallel()
138138
d := New(Config{})
139139
s := d.ipc
140140

141141
ic, client := newIPCTestConn(t)
142-
reply := runHandler(t, client, func() {
143-
s.handleSend(ic, 0, []byte{0x01, 0x02}) // only 2 bytes — need ≥4
144-
})
145-
assertErrorReply(t, reply, "send: missing conn_id")
142+
done := make(chan struct{})
143+
go func() { s.handleSend(ic, 0, []byte{0x01, 0x02}); close(done) }()
144+
assertNoReply(t, client) // fire-and-forget: no reply on error
145+
<-done
146146
}
147147

148-
// --- handleSend: unknown connID emits CmdError("connection N not found") ---
149-
150148
func TestHandleSendUnknownConnIDEmitsError(t *testing.T) {
151149
t.Parallel()
152150
d := New(Config{})
153151
s := d.ipc
154152

155153
ic, client := newIPCTestConn(t)
156-
// Valid 4-byte connID followed by payload; conn doesn't exist.
157154
payload := []byte{0x00, 0x00, 0xFF, 0xFE, 'x'}
158-
reply := runHandler(t, client, func() { s.handleSend(ic, 0, payload) })
159-
assertErrorReply(t, reply, "connection 65534 not found")
155+
done := make(chan struct{})
156+
go func() { s.handleSend(ic, 0, payload); close(done) }()
157+
assertNoReply(t, client) // fire-and-forget: no reply on error
158+
<-done
160159
}
161160

162161
// --- handleClose: unknown connID still writes CmdCloseOK (no error path) ---

pkg/daemon/zz_ipc_socket_lifecycle_test.go

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -238,10 +238,10 @@ func TestIPCServerHandleClientEmptyFrameContinuesLoop(t *testing.T) {
238238
}
239239
defer conn.Close()
240240

241-
// Send a frame too short for the envelope header (issue #99 wire
242-
// format), then a valid CmdBind. If the continue branch works, the
243-
// second frame still produces a CmdBindOK.
244-
if err := ipcutil.Write(conn, []byte{0x01, 0x02}); err != nil {
241+
// Send an empty frame (0 bytes — shorter than the 1-byte header),
242+
// then a valid CmdBind. If the continue branch works, the second
243+
// frame still produces a CmdBindOK.
244+
if err := ipcutil.Write(conn, []byte{}); err != nil {
245245
t.Fatalf("write short frame: %v", err)
246246
}
247247
if err := writeIPCRequest(conn, CmdBind, []byte{0x20, 0x01}); err != nil {

pkg/daemon/zz_ipc_test.go

Lines changed: 27 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -168,26 +168,40 @@ func TestHandleDialPortDeniedByPolicySendsError(t *testing.T) {
168168
assertErrorReply(t, reply, "not allowed")
169169
}
170170

171+
// assertNoReply verifies that no IPC frame is written within a short window.
172+
// handleSend and handleSendTo are fire-and-forget: they never write a reply.
173+
func assertNoReply(t *testing.T, client net.Conn) {
174+
t.Helper()
175+
_ = client.SetReadDeadline(time.Now().Add(50 * time.Millisecond))
176+
_, err := ipcutil.Read(client)
177+
if err == nil {
178+
t.Fatal("handler unexpectedly sent a reply")
179+
}
180+
}
181+
171182
// --- handleSend ---
172183

173184
func TestHandleSendShortPayloadSendsError(t *testing.T) {
174185
t.Parallel()
175186
d := New(Config{})
176187
s := d.ipc
177188
ic, client := newIPCTestConn(t)
178-
reply := runHandler(t, client, func() { s.handleSend(ic, 0, []byte{0x01}) }) // <4 bytes
179-
assertErrorReply(t, reply, "send: missing conn_id")
189+
done := make(chan struct{})
190+
go func() { s.handleSend(ic, 0, []byte{0x01}); close(done) }()
191+
assertNoReply(t, client) // fire-and-forget: no reply on error
192+
<-done
180193
}
181194

182195
func TestHandleSendUnknownConnIDSendsError(t *testing.T) {
183196
t.Parallel()
184197
d := New(Config{})
185198
s := d.ipc
186199
ic, client := newIPCTestConn(t)
187-
// 4-byte conn_id that doesn't exist + some payload.
188200
payload := []byte{0x00, 0x00, 0x00, 0x7B, 'a', 'b', 'c'} // conn_id 123
189-
reply := runHandler(t, client, func() { s.handleSend(ic, 0, payload) })
190-
assertErrorReply(t, reply, "connection 123 not found")
201+
done := make(chan struct{})
202+
go func() { s.handleSend(ic, 0, payload); close(done) }()
203+
assertNoReply(t, client) // fire-and-forget: no reply on error
204+
<-done
191205
}
192206

193207
// --- handleClose ---
@@ -227,8 +241,10 @@ func TestHandleSendToShortPayloadSendsError(t *testing.T) {
227241
d := New(Config{})
228242
s := d.ipc
229243
ic, client := newIPCTestConn(t)
230-
reply := runHandler(t, client, func() { s.handleSendTo(ic, 0, []byte{0x00}) })
231-
assertErrorReply(t, reply, "sendto: missing address/port")
244+
done := make(chan struct{})
245+
go func() { s.handleSendTo(ic, 0, []byte{0x00}); close(done) }()
246+
assertNoReply(t, client) // fire-and-forget: no reply on error
247+
<-done
232248
}
233249

234250
func TestHandleSendToBroadcastOnNetworkZeroSendsError(t *testing.T) {
@@ -244,8 +260,10 @@ func TestHandleSendToBroadcastOnNetworkZeroSendsError(t *testing.T) {
244260
binary.BigEndian.PutUint16(payload[protocol.AddrSize:], 80)
245261
copy(payload[protocol.AddrSize+2:], "data")
246262

247-
reply := runHandler(t, client, func() { s.handleSendTo(ic, 0, payload) })
248-
assertErrorReply(t, reply, "sendto:")
263+
done := make(chan struct{})
264+
go func() { s.handleSendTo(ic, 0, payload); close(done) }()
265+
assertNoReply(t, client) // fire-and-forget: no reply on error
266+
<-done
249267
}
250268

251269
// --- handleInfo ---

pkg/driver/driver.go

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -325,12 +325,15 @@ func (d *Driver) RotateKey() (map[string]interface{}, error) {
325325
}
326326

327327
// Disconnect closes a connection by ID. Used by administrative tools.
328+
// Fire-and-forget: the daemon always responds CmdCloseOK regardless of
329+
// whether the connID exists, so there is no error to propagate. Using
330+
// sendAndWait here would corrupt a concurrent sendAndWait for a different
331+
// command if a server-pushed cmdCloseOK (remote FIN) arrived simultaneously.
328332
func (d *Driver) Disconnect(connID uint32) error {
329333
msg := make([]byte, 5)
330334
msg[0] = cmdClose
331335
binary.BigEndian.PutUint32(msg[1:5], connID)
332-
_, err := d.ipc.sendAndWait(msg, cmdCloseOK)
333-
return err
336+
return d.ipc.send(msg)
334337
}
335338

336339
// NetworkList returns all networks known to the registry.

0 commit comments

Comments
 (0)