Skip to content

Commit

Permalink
Fix luv signals (issue ocaml-multicore#400)
Browse files Browse the repository at this point in the history
This makes sure we can process signals in luv the same way we do in uring. As
stated in ocaml-multicore#400 the main issue is that luv's mainloop will restart on EINTR and
we will never unwind back to ocaml land, so even though the process got the
signal, the runtime will not see it until something else goes on.

The trick here is to abuse POSIX thread semantics and shove all signals into one
specific systhread by blocking them in the other threads.

Additionally, I've fixed a bug in Ocaml 5.0 where the systhreads end up starting
with all signals blocked: ocaml/ocaml#11880. This PR
works even with/without the bug.

Danger Zone
~~~~~~~~~~~
This is tricky ! If you're thinking we can do better than pipes, think again !
Unix.sigsuspend doesn't work on multithreaded, we don't have a real Unix.pause
(it's implemented on top of sigsuspend !).
The semantics for kill(2) to "self" are different than "from outside" when
multithreaded, and the runtime doesn't expose pthread_kill(2). That's why on the
test we have to fork and signal back to the parent. I know, it's horrible.
  • Loading branch information
haesbaert committed Jan 23, 2023
1 parent edded87 commit e0b95a8
Show file tree
Hide file tree
Showing 2 changed files with 65 additions and 2 deletions.
34 changes: 32 additions & 2 deletions lib_eio_luv/eio_luv.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1186,7 +1186,7 @@ let rec wakeup ~async ~io_queued run_q =
Luv.Async.send async |> or_raise
| None -> ()

let rec run : type a. (_ -> a) -> a = fun main ->
let rec run2 : type a. (_ -> a) -> a = fun main ->
let loop = Luv.Loop.init () |> or_raise in
let run_q = Lf_queue.create () in
let io_queued = ref false in
Expand All @@ -1198,7 +1198,7 @@ let rec run : type a. (_ -> a) -> a = fun main ->
Luv.Loop.stop loop
) |> or_raise in
let st = { loop; async; run_q; fd_map = Fd_map.empty } in
let stdenv = stdenv ~run_event_loop:run in
let stdenv = stdenv ~run_event_loop:run2 in
let rec fork ~new_fiber:fiber fn =
Ctf.note_switch (Fiber_context.tid fiber);
let open Effect.Deep in
Expand Down Expand Up @@ -1304,3 +1304,33 @@ let rec run : type a. (_ -> a) -> a = fun main ->
| `Done v -> v
| `Ex (ex, bt) -> Printexc.raise_with_backtrace ex bt
| `Running -> failwith "Deadlock detected: no events scheduled but main function hasn't returned"

let start_signal_thread () =
let all = List.init 64 (fun x -> x) in
let omask = Thread.sigmask SIG_SETMASK all in
let inp, outp = Unix.pipe ~cloexec:true () in
let tid =
Thread.create
(fun () ->
Thread.sigmask SIG_SETMASK [] |> ignore;
let bytes = Bytes.create 1 in
let rec loop () =
match Unix.read inp bytes 0 1 with
| exception Unix.Unix_error (Unix.EINTR, _, _) -> loop ()
| 0 -> Unix.close inp
| _ -> failwith "signal pipe didn't return EOF or EINTR"
in
loop ()
) ()
in
tid, omask, outp

let stop_signal_thread (tid, omask, outp) =
Unix.close outp;
Thread.join tid;
Unix.sigprocmask SIG_SETMASK omask |> ignore

let run stdenv =
let sigctx = start_signal_thread () in
Fun.protect (fun () -> run2 stdenv)
~finally:(fun () -> stop_signal_thread sigctx)
33 changes: 33 additions & 0 deletions tests/signal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
# Setting up the environment

```ocaml
# #require "eio_main";;
```

# Test cases

Prove we can catch sigint:
```ocaml
# Eio_main.run @@ fun _stdenv ->
let got_sigint = ref false in
let old = Sys.signal Sys.sigint
(Signal_handle (fun _num -> got_sigint := true))
in
let ppid = Unix.getpid () in
let () = match Unix.fork () with
| 0 ->
Unix.kill ppid Sys.sigint;
Unix._exit 0
| child_pid ->
let wait () =
let pid, status = Unix.waitpid [] child_pid in
assert (pid = child_pid);
assert (status = (Unix.WEXITED 0))
in
try wait () with Unix.Unix_error (Unix.EINTR, _, _) -> wait ()
in
Eio.Std.traceln "got_sigint = %b" !got_sigint;
Sys.set_signal Sys.sigint old;;
+got_sigint = true
- : unit = ()
```

0 comments on commit e0b95a8

Please sign in to comment.