Skip to content

Commit d07f402

Browse files
authored
test(rpc): reproduce shutdown bugs (#14327)
Add expect tests covering the RPC shutdown issues found during review: - a server-initiated request that stays pending after the client disconnects - a client reply that raises while writing after the channel is closed - a client request that raises when sent after close Keep the server-side pending-request test focused by using a lenient test client that drops closed-write errors instead of failing first. Also annotate the remaining csexp RPC transport ownership bug in Client.connect: failed connects retain the socket until stop is called, and stop can still close the live session transport after a successful connect. Signed-off-by: Rudi Grinberg <me@rgrinberg.com>
1 parent 613cf95 commit d07f402

2 files changed

Lines changed: 136 additions & 1 deletion

File tree

src/rpc/csexp_rpc.ml

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -528,6 +528,14 @@ module Client = struct
528528
let backtrace = Printexc.get_callstack 10 in
529529
let transport = Transport.create t.sockaddr in
530530
let fd = transport.fd in
531+
(* CR-soon rgrinberg:
532+
533+
[t.transport] keeps owning [fd] across both failed and successful
534+
connects. That means callers must remember to [stop] a failed client to
535+
avoid leaking the socket, while calling [stop] after a successful
536+
[connect] can still close the live session transport. We should transfer
537+
ownership on success/failure or tighten the API so [stop] is only valid
538+
before [connect]. *)
531539
t.transport <- Some transport;
532540
Async_io.connect Socket.connect fd t.sockaddr
533541
>>| function

test/expect-tests/dune_rpc/finalization.ml

Lines changed: 128 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -201,7 +201,86 @@ let%expect_test "termination is always called" =
201201
|}]
202202
;;
203203

204-
let%expect_test "server-side pending request hangs on client disconnect" =
204+
let%expect_test "server-side pending request stays pending on disconnect" =
205+
let module Client = struct
206+
include
207+
Dune_rpc.Client.Make
208+
(Rpc.Private.Fiber)
209+
(struct
210+
include Chan
211+
212+
let write t packets =
213+
write t packets
214+
>>| function
215+
| Ok () -> ()
216+
| Error `Closed ->
217+
(* Keep this test focused on the server-side pending request bug.
218+
The default test client raises [Already_reported] here instead. *)
219+
()
220+
;;
221+
end)
222+
end
223+
in
224+
let upgraded_session = Fiber.Ivar.create () in
225+
let client_request_started = Fiber.Ivar.create () in
226+
let release_client_request = Fiber.Ivar.create () in
227+
let handler =
228+
let on_upgrade session _menu = Fiber.Ivar.fill upgraded_session session in
229+
let rpc = Handler.create ~on_init ~on_upgrade ~version:(1, 1) () in
230+
let () = Handler.declare_request rpc server_request_decl in
231+
rpc
232+
in
233+
let init =
234+
{ Initialize.Request.dune_version = 1, 1
235+
; protocol_version = Protocol.latest_version
236+
; id = Id.make (Atom "test-client")
237+
}
238+
in
239+
let run =
240+
let client_chan, sessions = setup_direct_client_server () in
241+
let client () =
242+
let private_menu =
243+
[ Client.Handle_request
244+
( server_request_decl
245+
, fun () ->
246+
let* () = Fiber.Ivar.fill client_request_started () in
247+
Fiber.Ivar.read release_client_request )
248+
]
249+
in
250+
Client.connect_with_menu client_chan init ~private_menu ~f:(fun _client ->
251+
let* () = Fiber.Ivar.read client_request_started in
252+
let* () =
253+
printfn "client: closing channel";
254+
Chan.close client_chan
255+
in
256+
Fiber.Ivar.fill release_client_request ())
257+
in
258+
let requester () =
259+
let* session = Fiber.Ivar.read upgraded_session in
260+
let id = Id.make (Atom "server-request") in
261+
let* () =
262+
Session.request session (Decl.Request.witness server_request_decl) id ()
263+
in
264+
printfn "server: request returned";
265+
Fiber.return ()
266+
in
267+
let server () =
268+
let+ () = Drpc.Server.serve sessions (Rpc.Server.make handler) in
269+
printfn "server: finished."
270+
in
271+
Fiber.parallel_iter [ client; requester; server ] ~f:(fun f -> f ())
272+
in
273+
let scheduler = Scheduler.create () in
274+
(try Scheduler.run scheduler run with
275+
| Scheduler.Never -> printfn "server: request remained pending after close");
276+
[%expect
277+
{|
278+
client: closing channel
279+
server: finished.
280+
server: request remained pending after close |}]
281+
;;
282+
283+
let%expect_test "client-side reply write raises on disconnect" =
205284
let upgraded_session = Fiber.Ivar.create () in
206285
let client_request_started = Fiber.Ivar.create () in
207286
let release_client_request = Fiber.Ivar.create () in
@@ -263,3 +342,51 @@ let%expect_test "server-side pending request hangs on client disconnect" =
263342
client: closing channel
264343
|}]
265344
;;
345+
346+
let%expect_test "client-side request write raises after close" =
347+
let handler =
348+
let rpc = Handler.create ~on_init ~version:(1, 1) () in
349+
let () = Handler.implement_request rpc decl (fun _ () -> Fiber.return ()) in
350+
rpc
351+
in
352+
let init =
353+
{ Initialize.Request.dune_version = 1, 1
354+
; protocol_version = Protocol.latest_version
355+
; id = Id.make (Atom "test-client")
356+
}
357+
in
358+
let run =
359+
let client_chan, sessions = setup_direct_client_server () in
360+
let client () =
361+
Drpc.Client.connect_with_menu
362+
client_chan
363+
init
364+
~private_menu:[ Request decl ]
365+
~f:(fun client ->
366+
let* () =
367+
printfn "client: closing channel";
368+
Chan.close client_chan
369+
in
370+
printfn "client: sending request";
371+
request_exn client witness ()
372+
>>| function
373+
| Error error -> print_dyn @@ Response.Error.to_dyn error
374+
| Ok _ -> assert false)
375+
in
376+
let server () =
377+
let+ () = Drpc.Server.serve sessions (Rpc.Server.make handler) in
378+
printfn "server: finished."
379+
in
380+
Fiber.parallel_iter [ client; server ] ~f:(fun f -> f ())
381+
in
382+
Scheduler.run (Scheduler.create ()) run;
383+
[%expect.unreachable]
384+
[@@expect.uncaught_exn
385+
{|
386+
(Dune_util__Report_error.Already_reported)
387+
Trailing output
388+
---------------
389+
client: closing channel
390+
client: sending request
391+
|}]
392+
;;

0 commit comments

Comments
 (0)