Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exit gracefully when a signal is received #1366

Merged
6 commits merged into from Oct 3, 2018
Merged

Exit gracefully when a signal is received #1366

6 commits merged into from Oct 3, 2018

Conversation

ghost
Copy link

@ghost ghost commented Oct 1, 2018

This PR modifies the scheduler to add unix signal as another source of events that are handled by the main thread. This way when the user presses Control+C the database is correctly saved to disk.

There is currently an issue with the file watcher: the at_exit handler waiting for the file watcher to terminate never terminates as it is the process watcher that receives the process status. The code needs to be refactored a bit to handle this.

@ghost ghost requested review from emillon and rgrinberg as code owners October 1, 2018 10:38
@nojb
Copy link
Collaborator

nojb commented Oct 1, 2018

Haven't had time to try this out yet, but what about killing all child processes when dune is killed? If memory serves, this was not the case some time ago (at least, under Windows).

@ejgallego
Copy link
Collaborator

ejgallego commented Oct 1, 2018

Tested, it works pretty well with Coq, thanks! I dunno tho if printing the message Got signal INT, exiting. is useful, maybe you could just exist silently [at least when display is not verbose].

@ghost
Copy link
Author

ghost commented Oct 2, 2018

@nojb, that seems good. However, this PR currently has no effect on Windows as it is using Thread.wait_signal. The way Dune handles external source of events is as follow: it spawns one system thread per source of events (processes termination, file changes, signals, ...) and each thread sends events to the main thread, which processes them synchronously. It should be easy to make this work on Windows using SetConsoleCtrlHandler given that the handler is executed in a separate thread, however it requires writing a bit of C code.

@ejgallego that seems good.

@ghost
Copy link
Author

ghost commented Oct 2, 2018

I changed the code to wait for processes on termination and not display the message by default. I also fixed the issue with the file watcher. This PR is ready for review.

@ghost
Copy link
Author

ghost commented Oct 2, 2018

@nojb I wrote a generic implementation using pipes and Sys.set_signal for Windows

@nojb
Copy link
Collaborator

nojb commented Oct 2, 2018

Great, I will give it a try tomorrow and report back.

src/scheduler.ml Outdated Show resolved Hide resolved
src/scheduler.ml Outdated

let init = lazy (
let wait_signal = signal_waiter () in
ignore (Thread.create run wait_signal : Thread.t))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you comment on as to why we can't just call wait_signal in the run thread.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because signals must be blocked before starting any thread, so that only the thread calling Thread.wait_signal may receive signals

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reworked the code to make this more explicit

src/scheduler.ml Outdated Show resolved Hide resolved
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
…reads

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@nojb
Copy link
Collaborator

nojb commented Oct 3, 2018

It doesn't seem to work on Windows (the signal handler is not called). I also tried using Sys.set_signal in a tiny test program under Windows but no luck so far.

@ghost
Copy link
Author

ghost commented Oct 3, 2018

Are you using the standard Windows terminal? Sys.set_signal Sys.sigint uses SetConsoleCtrlHandler, which I believe only works with the standard terminal application

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
@ghost
Copy link
Author

ghost commented Oct 3, 2018

I disabled handling of signals on Windows for now, I'm merging this PR since it improves the behavior on Unix.

@ghost ghost merged commit 6af98ea into ocaml:master Oct 3, 2018
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 4, 2018
CHANGES:

- Do not fail if the output of `ocamlc -config` doesn't include
  `standard_runtime` (ocaml/dune#1326, @diml)

- Let `Configurator.V1.C_define.import` handle negative integers
  (ocaml/dune#1334, @Chris00)

- Re-execute actions when a target is modified by the user inside
  `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml)

- Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml)

- Fix bad interaction between multi-directory libraries the `menhir`
  stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml)

- Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon)

- Better error message when using `(self_build_stubs_archive ...)` and
  `(c_names ...)` or `(cxx_names ...)` simultaneously.
  (ocaml/dune#1375, fix ocaml/dune#1306, @nojb)

- Improve name detection for packages when the prefix isn't an actual package
  (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg)

- Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg)

- Do not remove flags when compiling compatibility modules for wrapped mode
  (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg)

- Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc)

- Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`,
  `ocamlyacc` (ocaml/dune#1387, @diml)

- Exit gracefully when a signal is received (ocaml/dune#1366, @diml)

- Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344,
  @rgrinberg)

- Allow to use libraries `bytes`, `result` and `uchar` without `findlib`
  installed (ocaml/dune#1391, @nojb)
rgrinberg added a commit to rgrinberg/opam-repository that referenced this pull request Oct 10, 2018
CHANGES:

- Do not fail if the output of `ocamlc -config` doesn't include
  `standard_runtime` (ocaml/dune#1326, @diml)

- Let `Configurator.V1.C_define.import` handle negative integers
  (ocaml/dune#1334, @Chris00)

- Re-execute actions when a target is modified by the user inside
  `_build` (ocaml/dune#1343, fix ocaml/dune#1342, @diml)

- Pass `--set-switch` to opam (ocaml/dune#1341, fix ocaml/dune#1337, @diml)

- Fix bad interaction between multi-directory libraries the `menhir`
  stanza (ocaml/dune#1373, fix ocaml/dune#1372, @diml)

- Integration with automatic formatters (ocaml/dune#1252, fix ocaml/dune#1201, @emillon)

- Better error message when using `(self_build_stubs_archive ...)` and
  `(c_names ...)` or `(cxx_names ...)` simultaneously.
  (ocaml/dune#1375, fix ocaml/dune#1306, @nojb)

- Improve name detection for packages when the prefix isn't an actual package
  (ocaml/dune#1361, fix ocaml/dune#1360, @rgrinberg)

- Support for new menhir rules (ocaml/dune#863, fix ocaml/dune#305, @fpottier, @rgrinberg)

- Do not remove flags when compiling compatibility modules for wrapped mode
  (ocaml/dune#1382, fix ocaml/dune#1364, @rgrinberg)

- Fix reason support when using `staged_pps` (ocaml/dune#1384, @charlesetc)

- Add support for `enabled_if` in `rule`, `menhir`, `ocamllex`,
  `ocamlyacc` (ocaml/dune#1387, @diml)

- Exit gracefully when a signal is received (ocaml/dune#1366, @diml)

- Load all defined libraries recursively into utop (ocaml/dune#1384, fix ocaml/dune#1344,
  @rgrinberg)

- Allow to use libraries `bytes`, `result` and `uchar` without `findlib`
  installed (ocaml/dune#1391, @nojb)

- Take argument to self_build_stubs_archive into account. (ocaml/dune#1395, @nojb)

- Fix bad interaction between `env` customization and vendored
  projects: when a vendored project didn't have its own `env` stanza,
  the `env` stanza from the enclosing project was in effect (ocaml/dune#1408,
  @diml)

- Fix stop early bug when scanning for watermarks (ocaml/dune#1423, @diml)
shonfeder pushed a commit to shonfeder/dune that referenced this pull request Dec 31, 2018
Currently, this is only supported on Unix.

Signed-off-by: Jeremie Dimino <jeremie@dimino.org>
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants