-
Notifications
You must be signed in to change notification settings - Fork 270
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
ToolRunner.killChildProcess() unexpectedly sends SIGTERM instead of SIGINT #858
Comments
On second thought it's probably better to have this API mirror that of Node and have the same default, and leave it up to the caller to specify something else if desired. |
Hi @DaRosenberg, thank you for creating an issue. We're currently working on the more prioritized tickets. We'll take a look at it once we have enough capacity. |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
Issue is alive and well. |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
Issue is still alive and well. |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
Issue is alive and well. |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
Issue is alive and well. |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
not stale |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
Not stale, and PR remains available with an easy fix. |
Please someone review and approve this #859, without it tasks are not able to receive sigint properly. We are running terraform in bash task, and we only receive sigterm (with a proper bash trap), so terraform is not given enough time to gracefully shutdown and safe tfstate file. Again @DaRosenberg change is NOT breaking. It is fully backward compatible, which is great! |
This issue has had no activity in 90 days. Please comment if it is not actually stale |
Still a problem and a PR exists with an easy fix |
Hi @pszypowicz @DaRosenberg, signal parameter was added in this PR: #1008 |
Environment
azure-pipelines-task-lib version: 3.3.1
Issue Description
The method
ToolRunner.killChildProcess()
is documented as follows:However, its implementation forwards without arguments to Node's
ChildProcess.kill()
method (documented here) which by default sendsSIGTERM
to the child process unless a specific signal is specified.This leads to unexpected behavior for scripts in downstream
BashV3
pipeline task, where script tasks will receiveSIGTERM
instead ofSIGINT
.The bug was introduced with #663 the intent of which was to add functionality for killing child processes.
Fixing a related incorrect behavior in
BashV3
requires changes both here and in the downstream azure-pipelines-tasks library so I will create issues (and PRs) in both repos and update in both places to cross-reference.Expected behaviour
The
ToolRunner.killChildProcess()
method should have a parameter allowing caller to specify which signal should be sent to the child process. If undefined, the method should default toSIGINT
as per its docs and to ensure this is a non-breaking change.Actual behaviour
The
ToolRunner.killChildProcess()
method implicitly sendsSIGTERM
to child process.The text was updated successfully, but these errors were encountered: