Improve signal/interrupt handling
Howdy! I took a stab to fix multiple issues related to signal handling - and address some improvements on the current signal/interruption handler
https://github.com/casey/just/issues/1558 https://github.com/casey/just/issues/1560 https://github.com/casey/just/issues/1781
A summary of the changes:
- Renamed the
interrupt_handlertosignal_handlerto 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 withctrlc
- A substantial change was done to handle continuously polling the process via
- 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 https://github.com/casey/just/issues/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 :)
BTW, are you the Davo that I know IRL?
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!
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_signalfor windowsplatform.rspart - Refactored
usestatements - Cleaned up commented code
- Increased sleep time in between polling to 50ms
- Simplified Signal registering logic
- Changed
SignalHandler::instancemethod from public to private - Deleted
SignalHandler::newmethod - Fixed clippy comments, especially the one regarding conversions from u32 to i32
Rebased and took the liberty to resolve all trivial comments to make it less of a burden to review
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 🪟
Just re-ran the checks, it looks like the missing symbol errors are gone, but there are a few more issues.
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.
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
This looks a little stale, so I'm going to convert it to a draft.
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.