noseglid / atom-build

:hammer: Build your project directly from the Atom editor
https://atom.io/packages/build
MIT License
248 stars 97 forks source link

Send SIGINT rather than SIGTERM #452

Closed noseglid closed 8 years ago

noseglid commented 8 years ago

While SIGTERM may be the "correct" choice from a documentation standpoint, SIGINT is more widely used. Most applications are designed to run in a terminal and most (all?) send SIGINT when issuing ^C.

This PR simulates this behavior from atom-build.

oli-obk commented 8 years ago

:+1: wonderful. This probably fixes the issue that sometimes the running application isn't terminated if a new build is triggered before the old one finishes

noseglid commented 8 years ago

I hope so. Looks like with SIGTERM, the application should terminate as quickly as possible, and is allowed to leave some resources. Not good obviously. I think this will be a better approach

Gawdl3y commented 8 years ago

Perhaps a target config setting could be added to continue using SIGTERM, in case it's ever needed for some reason?

noseglid commented 8 years ago

@Gawdl3y perhaps if there ever arose a need for it. I highly doubt it though. To send SIGTERM in a terminal you'd have to invoke kill and find the pid. I'm willing to bet almost all programs respond "cleaner" to SIGINT.

noseglid commented 8 years ago

What might make sense is to progress this way for each press of Escape: SIGINT -> SIGTERM -> SIGKILL. They're defined with increasing "severity" - where the last one SIGKILL will not even give the process a chance to intercept. The OS will just take back all resources it's holding.

oli-obk commented 8 years ago

What would you do then in case the build is restarted before the previous one is finished?

noseglid commented 8 years ago

You can't. It will not start a new process until the previous one has been terminated. Issuing build should follow the same progression I think. First time, send SIGINT, when child terminates run build again. Once more and it will send SIGTERM and wait. Finally it's a SIGKILL which guarantees that it terminates and the build can be restarted.

noseglid commented 8 years ago

@oli-obk @Gawdl3y I'd love your input on this.

oli-obk commented 8 years ago

I tried to grok the code. But my javascript-closure-foo is not strong enough.

The idea seems fine to me, even though it'll mean my regular use case will be ESC + ESC + F6.

noseglid commented 8 years ago

That's a good point. For a lot of people I'd assume. Now I'm thinking I want to make it configurable, and keep the default at SIGTERM -> SIGKILL. Maybe in the build configuration even. On tors 30 juni 2016 at 16:04, Oliver Schneider notifications@github.com wrote:

I tried to grok the code. But my javascript-closure-foo is not strong enough.

The idea seems fine to me, even though it'll mean my regular use case will be ESC + ESC + F6.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/noseglid/atom-build/pull/452#issuecomment-229668194, or mute the thread https://github.com/notifications/unsubscribe/AA4_-DN3Hh8s54zwmgxC7Fnldk49BR-4ks5qQ8z5gaJpZM4I_1PK .

noseglid commented 8 years ago

I made it configurable on the build command. I think that makes sense, since commands may have different needs, so a global configuration might not cut it.

oli-obk commented 8 years ago

Awesome! Can it only be set from js or are the names strings?

Gawdl3y commented 8 years ago

Your solution definitely gets a :+1: from me.

noseglid commented 8 years ago

Works with any format. Build providers can also set it On lör 2 juli 2016 at 15:42, Oliver Schneider notifications@github.com wrote:

Awesome! Can it only be set from js or are the names strings?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/noseglid/atom-build/pull/452#issuecomment-230102278, or mute the thread https://github.com/notifications/unsubscribe/AA4_-JlAH8zoG52HYb35Ze4btNhJJknWks5qRmrJgaJpZM4I_1PK .

noseglid commented 8 years ago

Thanks for help and suggestion @oli-obk and @Gawdl3y