search

powder-rb / powder

Makes Pow even easier. I mean really, really, ridiculously easy
http://git.io/powder
MIT License
1.29k stars 69 forks source link

Use block to trap INT instead of trap. #108

Closed bradland closed 9 years ago

bradland commented 10 years ago

I was actually running powder 0.2.x when I noticed ^C caused an Interrupt exception when using powder log and powder app log. When I went to fork and submit a pull request, I noticed that SIGINT handling had already been implemented, but figured I'd send this over anyway. I, of course, understand completely if you decide to ignore this PR as well.

Everything I know about signal handling, I learned from Jessie Storimer. He is has been a tremendous asset as I write more and more Unix-friendly scripts! Hope you find this interesting/helpful! :) Really happy to have powder for managing my pow.

Signal::trap is global. Once it is called, the specified signal is always trapped, regardless of what code follows. Using a block ensures that the signal is only trapped at the point we need the behavior. This has very little impact in executables, because one wouldn't normally load or require this code elsewhere, but it affords us two opportunities:

  1. To avoid having any "infectious" code around, should someone copy/paste it elsewhere (all of the sudden the app prints "\nExiting log..." any time SIGINT is received).
  2. Allows us to provide an exit code that will play well with other shell scripts that might sniff for exit codes.

More info on exit codes: http://tldp.org/LDP/abs/html/exitcodes.html

philnash commented 9 years ago

Huh, that is interesting about the way that Signal::trap works. I had actually implemented this variety as well in #105. We had a bit of discussion about this in #104 which was the original issue. I'm happy to revisit this if there is a compelling reason to use the block syntax.

bradland commented 9 years ago

Yeah, it'd be hard to call this change "compelling" :) It matters most in libraries. There's nothing more frustrating than a library that calls Signal::trap within an api method, because thereafter the signal is handled however the library specified. Obvious conflicts can ensue.

Because this particular usage is in powder's executable, we never have to worry about the file being required or loaded elsewhere, hence, the trap of SIGINT only occurs while the powder log and applog tasks are running.

I went asking around a bit to check my math and someone provided this excellent write-up on the topic of signal handling in Ruby.

Anyway, this is just academic at this point :)

thomasklemm commented 9 years ago

:+1: I second this change and the 130 exit status. @bradland Thanks for opening a PR even for such detailed stuff.

philnash commented 9 years ago

Sorry for just returning to this now, I like the idea of the change and for the reasons for it. I particularly like the detail of the 130 exit code.

I'm going to merge this together with my original work in #105 to reduce duplication. Thanks!

bradland commented 9 years ago

Just cleaning up my old PRs. Thanks, everyone!