Improve signal/interrupt handling

[davoclavo]

Howdy! I took a stab to fix multiple issues related to signal handling - and address some improvements on the current signal/interruption handler

#1558
#1560
#1781

A summary of the changes:

• Renamed the interrupt_handler to signal_handler to make it more explicit that any arbitrary signal can be handled
• Replace ctrlc with signal-hook to be able to handle specific signals (such as SIGHUP) and be able to propagate them independently, instead of handling them all via a single handler
  • A substantial change was done to handle continuously polling the process via child.try_wait(), instead of handler that was used previously with ctrlc
• Fix error codes when recipes/commands/shebangs/inline are terminated via a signal by adding 128 to the signal termination code
• Add command-group to be able to handle shebang command executions which may cause sub-processes to be spawned
• Re-enable signal/interrupt tests and add a test that validates that multiple signals can be sent if the program is able to trap them

As a side note, I noticed that the current interruption handling mechanism was incrementing the block counter via the block() method, but was never calling unblock() thus decreasing the block counter - I assume this implementation was done with concurrent processes in mind, however it seemed to be incomplete. As far as I am aware there are no concurrent processes/recipes that would need this behaviour, but could definitely take it in mind if required.

Bonus:
Possibly re-fix this other reported issue a while back, which from my shallow understanding, currently multiple Ctrl-C's were not able to be forwarded to the child process due to the aforementioned block counter reason
#302

Please let me know if I should adjust any part of the implementation, more than happy to get some feedback and make appropriate adjustments :)

[casey]

Dope! Lots of comments, mostly because I don't fully understand what's going on here 😅

[casey]

BTW, are you the Davo that I know IRL?

[davoclavo]

Thanks a lot for the feedback/comments, I will work on fixing them! I will also address the issues clippy surfaced.

And yes, I am that davo from rc 😄 - stoked to see just and ords becoming big creatures!

[davoclavo]

Alright, comments adjusted, and made some additional improvements I noticed.

I did not mark any of your comments as "resolved" myself, as I am not sure what the right etiquette is for that process, but here is a list of the changes. I think I fixed all or most of your suggestions, and the ones I needed more clarification I left you a response in each thread. Happy to take more feedback if needed.

• Remove unwrap() calls
• Implement exit_code_from_signal for windows platform.rs part
• Refactored use statements
• Cleaned up commented code
• Increased sleep time in between polling to 50ms
• Simplified Signal registering logic
• Changed SignalHandler::instance method from public to private
• Deleted SignalHandler::new method
• Fixed clippy comments, especially the one regarding conversions from u32 to i32

[davoclavo]

Rebased and took the liberty to resolve all trivial comments to make it less of a burden to review

[casey]

I tweaked a few things and pushed them to the branch:

• Removed SignalHandler::verbosity since it looks like it we don't need it anymore
• Made SignalHandler::install set the whole struct and inlined some stuff
• Added #[ignore] back to the test, since for some reason they don't run when I run the tests inside cargo watch
• Made #[ignore]d tests run on CI
• Run ignored tests when running just test

It looks like this isn't working on Windows CI, there appear to be some undefined symbols.

[davoclavo]

Removed extended-siginfo feature from signal-hook crate which might be causing this issue, and we didn't really need it in the first place

Could you try running the workflow once again to see if tests pass for the Windows CI workflow?

if it fails, I'll do more research and try to get an easy way to replicate the problem and find a solution 🪟

[casey]

Just re-ran the checks, it looks like the missing symbol errors are gone, but there are a few more issues.

[davoclavo]

Pushed a couple more changes to address these new issues, hopefully all the CI checks pass this time 🤞 - thanks for the patience!

Also... I found what seems to be a libc::kill replacement for windows, so we may be able to enable the signal test for windows as well (modulo the test that uses SIGHUP, which apparently is not available in windows). Sadly I don't have a windows box handy to iterate and make sure things work, and have no clue how to set up a windows-latest github runner. I will try to set up one in my fork.

[davoclavo]

I got my windows runner working and there are some other issues, let me fix them and I will let you know when everything is green

[casey]

This looks a little stale, so I'm going to convert it to a draft.

[casey]

This is pretty stale, so closing. See #2473, where I try to get to the bottom, once and for all, of just's current signal handling behavior.