-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Handle signal termination in @actions/exec #2328
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| setTimeout(() => { | ||
| process.kill(process.pid, 'SIGTERM') | ||
| }, 50) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -501,15 +501,21 @@ export class ToolRunner extends events.EventEmitter { | |
| state.CheckComplete() | ||
| }) | ||
|
|
||
| cp.on('exit', (code: number) => { | ||
| cp.on('exit', (code: number | null, signal: NodeJS.Signals | null) => { | ||
| state.processExitCode = code | ||
| state.processSignal = signal | ||
| state.processExited = true | ||
| this._debug(`Exit code ${code} received from tool '${this.toolPath}'`) | ||
| if (signal) { | ||
| this._debug(`Signal ${signal} received from tool '${this.toolPath}'`) | ||
| } else { | ||
| this._debug(`Exit code ${code} received from tool '${this.toolPath}'`) | ||
| } | ||
| state.CheckComplete() | ||
| }) | ||
|
|
||
| cp.on('close', (code: number) => { | ||
| cp.on('close', (code: number | null, signal: NodeJS.Signals | null) => { | ||
| state.processExitCode = code | ||
| state.processSignal = signal | ||
| state.processExited = true | ||
| state.processClosed = true | ||
| this._debug(`STDIO streams have closed for tool '${this.toolPath}'`) | ||
|
|
@@ -625,7 +631,8 @@ class ExecState extends events.EventEmitter { | |
|
|
||
| processClosed = false // tracks whether the process has exited and stdio is closed | ||
| processError = '' | ||
| processExitCode = 0 | ||
| processExitCode: number | null = 0 | ||
| processSignal: NodeJS.Signals | null = null | ||
| processExited = false // tracks whether the process has exited | ||
| processStderr = false // tracks whether stderr was written to | ||
| private delay = 10000 // 10 seconds | ||
|
|
@@ -658,6 +665,10 @@ class ExecState extends events.EventEmitter { | |
| error = new Error( | ||
| `There was an error when attempting to execute the process '${this.toolPath}'. This may indicate the process failed to start. Error: ${this.processError}` | ||
| ) | ||
| } else if (this.processSignal) { | ||
| error = new Error( | ||
| `The process '${this.toolPath}' failed due to signal ${this.processSignal}` | ||
| ) | ||
| } else if (this.processExitCode !== 0 && !this.options.ignoreReturnCode) { | ||
|
Comment on lines
+668
to
672
|
||
| error = new Error( | ||
| `The process '${this.toolPath}' failed with exit code ${this.processExitCode}` | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new signal-termination error message text ("failed due to signal") doesn't match the newly added test expectation ("terminated by signal"). As-is, the test will fail; consider adjusting this message to include the wording the test asserts (or update the test to match the intended message).