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

Change default shell signal from SIGTERM to SIGINT #1392

Open
Tracked by #1391
chloeruka opened this issue Mar 5, 2021 · 2 comments
Open
Tracked by #1391

Change default shell signal from SIGTERM to SIGINT #1392

chloeruka opened this issue Mar 5, 2021 · 2 comments
Labels
breaking Changes to existing behaviour users might rely on

Comments

@chloeruka
Copy link
Contributor

chloeruka commented Mar 5, 2021

For historical reasons when we send interrupts to process groups, we default to using SIGTERM:

agent/process/signal.go

Lines 35 to 38 in b9bc5ec

// TODO: this should be SIGINT, but will be a breaking change
if intSignal == Signal(0) {
intSignal = SIGTERM
}

When we send SIGTERM to a process group, bash exits without waiting on its child processes to exit safely. In many cases, the child processes end from other effects, such as SIGPIPE caused by trying to read or write over an orphaned pipeline. This unreliability can lead to lost output or incorrect shutdowns of subprocesses. A script author could potentially work around this using a signal handler. A better approach would be to send a SIGINT signal instead, which will be followed by a SIGKILL if the group takes too long to respond. SIGINT only gets propagated to foreground child processes, so special case handling will still be required for users who are backgrounding processes.

Most processes treat SIGINT the same as SIGTERM, but we can't be sure if anyone is relying on this behaviour. Therefore, this should be considered a breaking change. A user can experiment with this change ahead of time by using the configurable cancel signals introduced by #1041 and #1390.

I've tried to carefully research this and get the above details correct, but please feel free to contribute any corrections or details I might have missed.

@chloeruka chloeruka added the breaking Changes to existing behaviour users might rely on label Mar 5, 2021
@pda
Copy link
Member

pda commented Mar 5, 2021

Interesting — have you come across any good links explaining the differences between SIGINT and SIGTERM?

My understanding over the years has stopped at “SIGINT is usually ctrl-c, SIGTERM is usually kill or program-initiated termination, SIGKILL is the kernel just no longer scheduling the process”. I never knew any difference with SIGINT vs SIGTERM around how it's propagated in progress groups between process leader / others processes and/r foreground processes. My understanding of process groups overall is pretty sketchy.

Got any links to things that explain SIGTERM vs SIGINT behaviour?

@chloeruka
Copy link
Contributor Author

My understanding of process groups overall is pretty sketchy.

Yeah, same! Learning this as I go. 😅

Interesting — have you come across any good links explaining the differences between SIGINT and SIGTERM?

This is cobbled together across various bits and pieces. I'll pop a few of the references I used down:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Changes to existing behaviour users might rely on
Projects
None yet
Development

No branches or pull requests

2 participants