Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send SIGINT rather than SIGTERM #452

Merged
merged 5 commits into from
Jul 5, 2016
Merged

Send SIGINT rather than SIGTERM #452

merged 5 commits into from
Jul 5, 2016

Conversation

noseglid
Copy link
Owner

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.

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
Copy link
Contributor

oli-obk commented Jun 28, 2016

👍 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
Copy link
Owner Author

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
Copy link
Contributor

Gawdl3y commented Jun 28, 2016

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

@noseglid
Copy link
Owner Author

@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
Copy link
Owner Author

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
Copy link
Contributor

oli-obk commented Jun 29, 2016

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

@noseglid
Copy link
Owner Author

noseglid commented Jun 29, 2016

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.

Build will now start of by sending SIGINT (similar to what
many terminals do). If the process hasn't terminated, it will
send SIGTERM on the next press. Finally SIGKILL is issued which
guarantees process termination.

The same is true if issuing a new build while another build
is running.
@noseglid
Copy link
Owner Author

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

@oli-obk
Copy link
Contributor

oli-obk commented Jun 30, 2016

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
Copy link
Owner Author

noseglid commented Jun 30, 2016

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
#452 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA4_-DN3Hh8s54zwmgxC7Fnldk49BR-4ks5qQ8z5gaJpZM4I_1PK
.

@noseglid
Copy link
Owner Author

noseglid commented Jul 2, 2016

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
Copy link
Contributor

oli-obk commented Jul 2, 2016

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

@Gawdl3y
Copy link
Contributor

Gawdl3y commented Jul 2, 2016

Your solution definitely gets a 👍 from me.

@noseglid
Copy link
Owner Author

noseglid commented Jul 2, 2016

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
#452 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AA4_-JlAH8zoG52HYb35Ze4btNhJJknWks5qRmrJgaJpZM4I_1PK
.

@noseglid noseglid merged commit 0e9cc5c into master Jul 5, 2016
@noseglid
Copy link
Owner Author

noseglid commented Jul 5, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants