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

Ability to execute code upon SIGINT and SIGTERM signals. #1262

Closed
amano-kenji opened this issue Aug 19, 2023 · 24 comments
Closed

Ability to execute code upon SIGINT and SIGTERM signals. #1262

amano-kenji opened this issue Aug 19, 2023 · 24 comments

Comments

@amano-kenji
Copy link
Contributor

defer and edefer do not handle SIGINT and SIGTERM.

I'd like to handle these signals in my programs.

@bakpakin
Copy link
Member

Related: #1030

We don't have support for signal handlers in the core. Given the continued addition of posix only functionality, and the addition of a signal argument to os/proc-kill, this would be a good addition.

@sogaiu
Copy link
Contributor

sogaiu commented Aug 19, 2023

Perhaps known already, but for reference, there was some related work here: https://github.com/andrewchambers/janet-ctrl-c

@bakpakin
Copy link
Member

bakpakin commented Aug 20, 2023

Pushed to the sigaction branch with an initial implementation of (os/sigaction). This is a generalization of the work from spork/rawterm used for handling SIGWINCH.

Still hashing out the exact interface though, and signals should play well with the rest of Janet (threads, sandbox, ev, busy loops, etc.). I think a binding for sigprocmask/pthread_sigmask may also be warranted.

@sogaiu
Copy link
Contributor

sogaiu commented Aug 20, 2023

So it looks like there is an example.

Adapting that:

(defn action []
  (print "Handled SIGINT, waiting 5 seconds before...")
  (os/sleep 5)
  (os/exit 1))

(defn main
  [_]
  (os/sigaction :int action true)
  (forever))

Trying it out:

$ ./build/janet ~/scratch/sigaction-sigint.janet 

Followed by Ctrl-C:

^CHandled SIGINT, waiting 5 seconds before...

After a brief wait I got a prompt and then:

$ echo $?
1

@amano-kenji Is it feasible for you to give the code a try to see if it works for the kind of situation you have in mind?

@amano-kenji
Copy link
Contributor Author

I just tried to use os/sigaction. The interface isn't perfect, but this is good enough.

There is one little problem which is lack of proper documentation.

If (doc os/sigaction) is more properly documented, I think this issue can be closed.

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Aug 22, 2023

I found one issue.

(defn action []
  (print "Handled SIGINT!")
  (os/exit 1))

(defn main [_]
  # Set the interrupt-interpreter argument to `true` to allow
  # interrupting the busy loop `(forever)`. By default, will not
  # interrupt the interpreter.
  (os/sigaction :int action true)
  (forever))

I can't close this script with Ctrl+c. The script is stuck (forever).

os/sigaction also doesn't respond to :term signal.

@sogaiu
Copy link
Contributor

sogaiu commented Aug 22, 2023

I didn't have luck with SIGTERM / :term either.

However, the posted sample script (for SIGINT / :int) seems to work here (Ubuntu Linux):

$ cat ak.janet 
(defn action []
  (print "Handled SIGINT!")
  (os/exit 1))

(defn main [_]
  # Set the interrupt-interpreter argument to `true` to allow
  # interrupting the busy loop `(forever)`. By default, will not
  # interrupt the interpreter.
  (os/sigaction :int action true)
  (forever))

$ janet ak.janet 
^CHandled SIGINT!

$ janet -v
1.30.0-ee01045d

@amano-kenji
Copy link
Contributor Author

I'm on gentoo linux.

@primo-ppcg
Copy link
Contributor

However, the posted sample script (for SIGINT / :int) seems to work here (Linux):

Also works on Debian 12.

@sogaiu
Copy link
Contributor

sogaiu commented Aug 22, 2023

The script also worked with Void Linux and NetBSD (janet commit: 70a467d).

@bakpakin
Copy link
Member

SIGINT and SIGTERM both work for me. Btw, I don't recommend using the interrupt interpreter example in most cases, instead it is better to change the busy loop to something like (forever (ev/sleep 100)). The interrupt interpreter is sort of a last resort to make sure cleanup code runs (so I guess makes sense for sigint?), but doing anything safely is hard because the current executing fiber can be interrupted at any random instruction instead of the usual places where control is yielded to the event loop

@sogaiu
Copy link
Contributor

sogaiu commented Aug 23, 2023

SIGINT and SIGTERM both work for me.

I've tried the following for SIGTERM and I haven't had success so far. The script exits and I see no message printed.

(defn action
  []
  (print "Handled SIGTERM!")
  (flush)
  (os/sleep 1000)
  (os/exit 1))

(defn main
  [_]
  (os/sigaction :term action true)
  (forever (ev/sleep 100)))

Does the code look problematic? Or perhaps the behavior is explained by:

The interrupt interpreter is sort of a last resort to make sure cleanup code runs (so I guess makes sense for sigint?), but doing anything safely is hard because the current executing fiber can be interrupted at any random instruction instead of the usual places where control is yielded to the event loop

@amano-kenji
Copy link
Contributor Author

What is interrupt interpreter?

@sogaiu
Copy link
Contributor

sogaiu commented Aug 23, 2023

Possibly some hints from these lines of the example:

  # Set the interrupt-interpreter argument to `true` to allow
  # interrupting the busy loop `(forever)`. By default, will not
  # interrupt the interpreter.

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Aug 23, 2023

According to my experiment, these cannot be interrupted by signal handler.

  • (forever)
  • (while true)
  • (forever (os/sleep 1))
  • (while true (os/sleep 1))

But, os/sigaction can handle

  • (while true (ev/sleep 100))
  • (forever (ev/sleep 100))

@bakpakin
Copy link
Member

So the interrupt-interpreter option will only interrupt the Janet virtual machine, not arbitrary machine code. Basically, if you have a loop like:

(var a 0)
(forever
  (++ a))

this loop will block events from being handled forever (unless you manually add something like (ev/sleep 0) inside the loop). The interrupt interpreter argument sets a flag that Janet checks on all backwards jumps and function calls that will automatically force it to yield to the event loop to handle events, like a signal. However, this might break some guarantees about your code executing atomically unless you are clever/careful to avoid backwards jumps and function calls in code that needs to be "atomic" in the interpreter.

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Aug 23, 2023

Python has it. Haskell has it. Raku has it. Why is it dangerous in janet?

Is it still safe to interrupt ev/sleep?

bakpakin added a commit that referenced this issue Aug 23, 2023
Update example to have 4 cases - case 3 was previously broken but should
now work.
@bakpakin
Copy link
Member

It's not "dangerous", it is perfectly memory safe either way. It's just not usually needed and can lead to confusing behavior, and things might not be handled in the order that you expect.

I've added a more complete example that has 4 cases the should help illustrate what is going on here. Python's signals do not interact well with asyncio, and I don't know anything about Raku or Haskell here.

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Aug 24, 2023

I just read 21eab7e

main1 doesn't work when I send it TERM signal. But, it quits upon SIGINT.

It seems only ev/sleep is interruptible by os/sigaction.

@bakpakin
Copy link
Member

Works on my machine ™️ . Anyway, that is strange, I'm going to let this feature cook a little more and see if I can make it work better / clean-up the scheduling code to be a bit more straight-forward. If it is not satisfactory I may need to remove the interpreter interrupt functionality but I think it is a useful tool to have.

@sogaiu
Copy link
Contributor

sogaiu commented Aug 24, 2023

I got the expected results:

  • main1, main3, and main4 worked
  • main2 did not work

So it seems to be working as advertised here.

I used kill -TERM <pid> to test. Is anyone using a different method to test?

I wasn't doing so much earlier in the issue and perhaps that explains why it seemed SIGTERM wasn't working for me (when perhaps it might have).

@amano-kenji
Copy link
Contributor Author

I just read meson_options.txt which has

option('interpreter_interrupt', type : 'boolean', value : false)

So, interpreter interrupt is disabled by default.

@amano-kenji
Copy link
Contributor Author

amano-kenji commented Aug 24, 2023

After executing

meson setup build -Dinterpreter_interrupt=true ...
ninja -C build

examples/sigaction.janet works as expected. The following code also works.

(defn handler
  []
  (print "Handled SIGINT")
  (os/exit 1))

(defn main
  [_]
  (os/sigaction :int handler true)
  (forever))

Lesson: Don't mess with meson.

bakpakin added a commit that referenced this issue Aug 24, 2023
Instead of setting a flag, each interrupt increments an atomic
counter. When the interrupt is finally handled, either by scheduling
code to run on the event loop or executing some out of band code, the
user must now decrement the interrupt counter with
janet_interpreter_interrupt_handled. While this counter is non-zero, the
event loop will not enter the interpreter. This changes the API a bit but
makes it possible and easy to handle signals without race conditions
or scheduler hacks, as the runtime can ensure that high priority code is
run before re-entering possibly blocking interpreter code again.

Also included is a new function janet_schedule_soon, which prepends to
the task queue instead of appending, allowing interrupt handler to skip
ahead of all other scheduled fibers.

Lastly, also update meson default options to include the
interpreter_interrupt code and raise a runtime error if os/sigaction
is used with interpreter interrupt but that build option is not enabled.
@bakpakin
Copy link
Member

Made some improvements to the implementation and fixed the default meson build options. Closing this as now implemented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants