Ensure that restore_terminal is called on sigint

[outkine]

re #93

[ulrikstrid]

Do we really want exit 1?

[aantron]

Do we really want exit 1?

@ulrikstrid Can you be more precise with your objection? Why wouldn't we want exit 1? What's the alternative (or alternatives)?

Dream.run is used for the top level of relatively simple Dream programs. Most of these will have SIGINT set to Signal_default, which will terminate the process with some non-zero exit code (130 on my platform). How is exit 1 substantially different from this?

The only thing I would suggest, perhaps for a separate PR, is to use Sys.signal, store the previous behavior in a ref (because the API is awkward and you can only get the previous behavior after you have already set the new one), and then trigger the previous behavior by dereferencing the ref. If the previous behavior was Signal_handle, call the function. If it was Signal_ignore, do nothing. If it was Signal_default, call exit 1. @outkine would you mind making this change during this PR? It would extend the usefulness of Dream.run for people that want to set a custom signal handler — we would no longer blindly override it, as done by this PR (though it is an improvement w.r.t. #93).

[aantron]

The only other thing I can think of is that 1 might be a bad choice for a Ctrl+C exit code. @ulrikstrid or @outkine, if you are on Mac, what do you get as the exit code for Ctrl+C from just any other OCaml program, or a Dream example without this PR yet applied?

[aantron]

Actually, if it works, a better approach might be to use at_exit rather than a signal handler and the normal-exit code that is already in Dream.run. However, the documentation does not mention that this function runs in response to uncaught signals, so that would have to be checked.

[outkine]

at_exit does not appear to react to ctrl-c. I don't have a Mac so I'm not sure about the exit code, but I'll make the Sys.signal change.

[outkine]

Would it make sense to handle sigkill as well?

[aantron]

Would it make sense to handle sigkill as well?

Yes, please.

[aantron]

Try combining this with the next line with

let rec prev_signal_behavior := Sys.signal ...

Not sure if it will work.

[outkine]

Nope, that's a syntax error

[aantron]

Ah yes, I meant

let rec prev_signal_behavior = ref (Sys.signal ...

[outkine]

Ah, but is that possible with the new structure?

let prev_signal_behavior = ref Sys.Signal_default in

let handler () = (Sys.Signal_handle (fun signal ->
  restore_terminal ();
  match !prev_signal_behavior with
    | Sys.Signal_default -> exit 1
    | Sys.Signal_handle f -> f signal
    | Sys.Signal_ignore -> ignore ()
)) in

prev_signal_behavior := Sys.signal Sys.sigint @@ handler ();
prev_signal_behavior := Sys.signal Sys.sigterm @@ handler ();

[aantron]

This new structure is incorrect AFAIK, as the derferencing of prev_signal_behavior is suspended inside closures (fun signal -> ...), which are only called upon actual reception of signals. By then, the setup code will have assigned prev_signal_behavior twice, the second time for SIGTERM, completely losing the SIGINT previous behavior, and making both signals behave as the previous behavior of SIGTERM.

By far the easiest way to get this code right (and to make it immediately legible for readers) is to use a local reference for each signal, and you will probably be able to use the rec in that case, too.

[outkine]

I'm not exactly sure what you mean by using "a local reference for each signal". It seems to me that the ref has to be passed as an argument to some sort of create_handler function, no?

let create_handler prev_signal_behavior = (Sys.Signal_handle (fun signal ->
  restore_terminal ();
  match !prev_signal_behavior with
    | Sys.Signal_default -> exit 1
    | Sys.Signal_handle f -> f signal
    | Sys.Signal_ignore -> ignore ()
)) in

let rec sigint_behavior = ref (Sys.signal Sys.sigint @@ handler sigint_behavior);

Except it looks like this rec is also not possible, I'm getting a compilation error:

This kind of expression is not allowed as right-hand side of `let rec'ocamllsp

[aantron]

No, the ref can and really should be created inside the create_handler function, which guarantees that it will be local to that handler. create_handler should take a signal number, not a ref to a signal behavior.

[outkine]

Would it make sense to handle sigkill as well?

Yes, please.

Actually, it looks like it's not possible. Setting any handler for Sys.sigkill throws:

Fatal error: exception Sys_error("Invalid argument")

[aantron]

Would it make sense to handle sigkill as well?

Yes, please.

Actually, it looks like it's not possible. Setting any handler for Sys.sigkill throws:

Fatal error: exception Sys_error("Invalid argument")

Try SIGTERM instead. One or both of us may have confused the signals. SIGKILL is, looking again, the uncatchable one.

[aantron]

Looks good now, thanks!