Skip to content

Commit

Permalink
fix(core): fix sending sigint to child tasks with the new psuedo tty … (
Browse files Browse the repository at this point in the history
#21369)

Co-authored-by: Jonathan Cammisuli <[email protected]>
  • Loading branch information
FrozenPandaz and Cammisuli authored Jan 27, 2024
1 parent e1bb8bc commit 00dbd14
Show file tree
Hide file tree
Showing 9 changed files with 90 additions and 37 deletions.
4 changes: 3 additions & 1 deletion .prettierignore
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ packages/nx/src/plugins/js/lock-file/__fixtures__/**/*.*
packages/**/schematics/**/files/**/*.html
packages/**/generators/**/files/**/*.html
packages/nx/src/native/**/*.rs
packages/nx/src/native/index.js
packages/nx/src/native/index.d.ts
nx-dev/nx-dev/.next/
nx-dev/nx-dev/public/documentation
graph/client/src/assets/environment.js
Expand All @@ -37,4 +39,4 @@ CODEOWNERS

/.nx/cache

.pnpm-store
.pnpm-store
15 changes: 13 additions & 2 deletions packages/nx/src/executors/run-commands/run-commands.impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { env as appendLocalEnv } from 'npm-run-path';
import { ExecutorContext } from '../../config/misc-interfaces';
import * as chalk from 'chalk';
import { runCommand } from '../../native';
import { PseudoTtyProcess } from '../../utils/child-process';

export const LARGE_BUFFER = 1024 * 1000000;

Expand Down Expand Up @@ -230,7 +231,9 @@ async function createProcess(
!commandConfig.prefix &&
!isParallel
) {
const cp = runCommand(commandConfig.command, cwd, env);
const cp = new PseudoTtyProcess(
runCommand(commandConfig.command, cwd, env)
);

return new Promise((res) => {
cp.onOutput((output) => {
Expand All @@ -239,7 +242,15 @@ async function createProcess(
}
});

cp.onExit((code) => res(code === 0));
cp.onExit((code) => {
if (code === 0) {
res(true);
} else if (code >= 128) {
process.exit(code);
} else {
res(false);
}
});
});
}

Expand Down
26 changes: 16 additions & 10 deletions packages/nx/src/executors/run-script/run-script.impl.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import type { ExecutorContext } from '../../config/misc-interfaces';
import * as path from 'path';
import { env as appendLocalEnv } from 'npm-run-path';
import { runCommand } from '../../native';
import { PseudoTtyProcess } from '../../utils/child-process';
import { signalToCode } from '../../utils/exit-codes';

export interface RunScriptOptions {
script: string;
Expand All @@ -16,20 +18,24 @@ export default async function (
const pm = getPackageManagerCommand();
try {
await new Promise<void>((res, rej) => {
const cp = runCommand(
pm.run(options.script, options.__unparsed__.join(' ')),
path.join(
context.root,
context.projectsConfigurations.projects[context.projectName].root
),
{
...process.env,
...appendLocalEnv(),
}
const cp = new PseudoTtyProcess(
runCommand(
pm.run(options.script, options.__unparsed__.join(' ')),
path.join(
context.root,
context.projectsConfigurations.projects[context.projectName].root
),
{
...process.env,
...appendLocalEnv(),
}
)
);
cp.onExit((code) => {
if (code === 0) {
res();
} else if (code >= 128) {
process.exit(code);
} else {
rej();
}
Expand Down
13 changes: 6 additions & 7 deletions packages/nx/src/native/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@ pub enum ChildProcessMessage {
pub struct ChildProcess {
process_killer: Box<dyn ChildKiller + Sync + Send>,
message_receiver: Receiver<String>,
wait_receiver: Receiver<u32>,
wait_receiver: Receiver<String>,
}
#[napi]
impl ChildProcess {
pub fn new(
process_killer: Box<dyn ChildKiller + Sync + Send>,
message_receiver: Receiver<String>,
exit_receiver: Receiver<u32>,
exit_receiver: Receiver<String>,
) -> Self {
Self {
process_killer,
Expand All @@ -57,17 +57,16 @@ impl ChildProcess {

#[napi]
pub fn kill(&mut self) -> anyhow::Result<()> {
self.process_killer.kill().map_err(anyhow::Error::from)?;
Ok(())
self.process_killer.kill().map_err(anyhow::Error::from)
}

#[napi]
pub fn on_exit(
&mut self,
#[napi(ts_arg_type = "(code: number) => void")] callback: JsFunction,
#[napi(ts_arg_type = "(message: string) => void")] callback: JsFunction,
) -> napi::Result<()> {
let wait = self.wait_receiver.clone();
let callback_tsfn: ThreadsafeFunction<u32, Fatal> =
let callback_tsfn: ThreadsafeFunction<String, Fatal> =
callback.create_threadsafe_function(0, |ctx| Ok(vec![ctx.value]))?;

std::thread::spawn(move || {
Expand Down Expand Up @@ -205,7 +204,7 @@ pub fn run_command(
// make sure that master is only dropped after we wait on the child. Otherwise windows does not like it
drop(pair.master);
disable_raw_mode().expect("Failed to restore non-raw terminal");
exit_tx.send(exit.exit_code()).ok();
exit_tx.send(exit.to_string()).ok();
});

Ok(ChildProcess::new(process_killer, message_rx, exit_rx))
Expand Down
2 changes: 1 addition & 1 deletion packages/nx/src/native/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ export interface FileMap {
export function testOnlyTransferFileMap(projectFiles: Record<string, Array<FileData>>, nonProjectFiles: Array<FileData>): NxWorkspaceFilesExternals
export class ChildProcess {
kill(): void
onExit(callback: (code: number) => void): void
onExit(callback: (message: string) => void): void
onOutput(callback: (message: string) => void): void
}
export class ImportResult {
Expand Down
6 changes: 2 additions & 4 deletions packages/nx/src/native/tests/command.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('runCommand', () => {
});

childProcess.onExit(() => {
expect(output.trim()).toEqual('hello world');
expect(output.trim()).toContain('hello world');
done();
});
});
Expand All @@ -41,9 +41,7 @@ describe('runCommand', () => {
// check to make sure that we have ansi sequence characters only available in tty terminals
});
childProcess.onExit((_) => {
expect(JSON.stringify(output)).toMatchInlineSnapshot(
`""\\u001b[33mtrue\\u001b[39m""`
);
expect(JSON.stringify(output)).toContain('\\u001b[33mtrue\\u001b[39m');
done();
});
});
Expand Down
21 changes: 10 additions & 11 deletions packages/nx/src/tasks-runner/forked-process-task-runner.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { ChildProcess as NativeChildProcess, nxFork } from '../native';
import { PsuedoIPCServer } from './psuedo-ipc';
import { FORKED_PROCESS_OS_SOCKET_PATH } from '../daemon/socket-utils';
import { PseudoTtyProcess } from '../utils/child-process';
import { signalToCode } from '../utils/exit-codes';

const forkScript = join(__dirname, './fork.js');

Expand Down Expand Up @@ -71,7 +72,7 @@ export class ForkedProcessTaskRunner {

p.once('exit', (code, signal) => {
this.processes.delete(p);
if (code === null) code = this.signalToCode(signal);
if (code === null) code = signalToCode(signal);
if (code !== 0) {
const results: BatchResults = {};
for (const rootTaskId of batchTaskGraph.roots) {
Expand Down Expand Up @@ -236,6 +237,11 @@ export class ForkedProcessTaskRunner {

return new Promise((res) => {
p.onExit((code) => {
// If the exit code is greater than 128, it's a special exit code for a signal
if (code >= 128) {
process.exit(code);
}

res({
code,
terminalOutput,
Expand Down Expand Up @@ -339,7 +345,7 @@ export class ForkedProcessTaskRunner {

p.on('exit', (code, signal) => {
this.processes.delete(p);
if (code === null) code = this.signalToCode(signal);
if (code === null) code = signalToCode(signal);
// we didn't print any output as we were running the command
// print all the collected output|
const terminalOutput = outWithErr.join('');
Expand Down Expand Up @@ -403,7 +409,7 @@ export class ForkedProcessTaskRunner {
});

p.on('exit', (code, signal) => {
if (code === null) code = this.signalToCode(signal);
if (code === null) code = signalToCode(signal);
// we didn't print any output as we were running the command
// print all the collected output
let terminalOutput = '';
Expand Down Expand Up @@ -445,13 +451,6 @@ export class ForkedProcessTaskRunner {
writeFileSync(outputPath, content);
}

private signalToCode(signal: string) {
if (signal === 'SIGHUP') return 128 + 1;
if (signal === 'SIGINT') return 128 + 2;
if (signal === 'SIGTERM') return 128 + 15;
return 128;
}

private setupProcessEventListeners() {
this.psuedoIPC.onMessageFromChildren((message: Serializable) => {
process.send(message);
Expand Down Expand Up @@ -484,7 +483,7 @@ export class ForkedProcessTaskRunner {
}
});
// we exit here because we don't need to write anything to cache.
process.exit(this.signalToCode('SIGINT'));
process.exit(signalToCode('SIGINT'));
});
process.on('SIGTERM', () => {
this.processes.forEach((p) => {
Expand Down
24 changes: 23 additions & 1 deletion packages/nx/src/utils/child-process.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,14 +76,36 @@ export async function runNxAsync(
});
}

function messageToCode(message: string): number {
if (message.startsWith('Terminated by ')) {
switch (message.replace('Terminated by ', '').trim()) {
case 'Termination':
return 143;
case 'Interrupt':
return 130;
default:
return 128;
}
} else if (message.startsWith('Exited with code ')) {
return parseInt(message.replace('Exited with code ', '').trim());
} else if (message === 'Success') {
return 0;
} else {
return 1;
}
}

export class PseudoTtyProcess {
isAlive = true;

exitCallbacks = [];

constructor(private childProcess: ChildProcess) {
childProcess.onExit((exitCode) => {
childProcess.onExit((message) => {
this.isAlive = false;

const exitCode = messageToCode(message);

this.exitCallbacks.forEach((cb) => cb(exitCode));
});
}
Expand Down
16 changes: 16 additions & 0 deletions packages/nx/src/utils/exit-codes.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
/**
* Translates NodeJS signals to numeric exit code
* @param signal
*/
export function signalToCode(signal: NodeJS.Signals): number {
switch (signal) {
case 'SIGHUP':
return 128 + 1;
case 'SIGINT':
return 128 + 2;
case 'SIGTERM':
return 128 + 15;
default:
return 128;
}
}

0 comments on commit 00dbd14

Please sign in to comment.