just icon indicating copy to clipboard operation
just copied to clipboard

Improve signal/interrupt handling

Open davoclavo opened this issue 1 year ago • 9 comments

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_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 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 :)

davoclavo avatar Dec 30 '23 05:12 davoclavo

BTW, are you the Davo that I know IRL?

casey avatar Jan 02 '24 01:01 casey

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 avatar Jan 02 '24 02:01 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 avatar Jan 02 '24 04:01 davoclavo

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

davoclavo avatar Jan 12 '24 04:01 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 🪟

davoclavo avatar Jan 12 '24 05:01 davoclavo

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

casey avatar Jan 12 '24 06:01 casey

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 avatar Jan 12 '24 06:01 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

davoclavo avatar Jan 12 '24 06:01 davoclavo

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

casey avatar May 21 '24 00:05 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.

casey avatar Nov 19 '24 21:11 casey