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

Improper handling of graceful exits #95

Closed
1 task
gz-95 opened this issue Sep 2, 2022 · 3 comments · Fixed by #116
Closed
1 task

Improper handling of graceful exits #95

gz-95 opened this issue Sep 2, 2022 · 3 comments · Fixed by #116
Labels
bug Something isn't working outdated pr welcome

Comments

@gz-95
Copy link

gz-95 commented Sep 2, 2022

Bug description

tsx does not wait for script to exit gracefully before terminating the process.

Reproduction

Let's say you have main.ts as the following:

console.log('starting up');

process.on('SIGINT', () => {
  console.log('shutting down');

  // simulate cleanup
  setTimeout(() => {
    console.log('cleanly exiting');
    process.exit(0);
  }, 5000);
});

// run forever
setInterval(() => {}, 1 << 30);

Running main.ts via node --no-warnings --loader tsx main.ts will produce the correct behavior, i.e. pressing CTRL+C will signal the script to start cleaning up and then exit gracefully.

On the other hand, when running via tsx main.ts, the process will exit abruptly when signaled without waiting for the script to completely finish cleaning up.

Environment

System:
    OS: Linux 5.15 Ubuntu 22.04.1 LTS 22.04.1 LTS (Jammy Jellyfish)
    CPU: (4) x64 Intel(R) Core(TM) i7-7500U CPU @ 2.70GHz
    Memory: 10.00 GB / 15.52 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 18.7.0 - /usr/local/node/bin/node
    npm: 8.15.0 - /usr/local/node/bin/npm
  npmPackages:
    tsx: ^3.8.2 => 3.8.2

Can you contribute a fix?

  • I’m interested in opening a pull request for this issue.
@privatenumber
Copy link
Owner

privatenumber commented Oct 12, 2022

I investigated this and turns out it's a really difficult problem. I'm currently convinced there isn't a good solution in Node.js.

Here are some approaches I considered:

1. Relay kill signals from parent to child process

import { constants } from 'os'

const uncatchableSignals = ['SIGKILL', 'SIGSTOP']

for (const signal of Object.keys(constants.signals)) {
	if (!uncatchableSignals.includes(signal)) {
		process.on(signal, () => {
			child.kill(signal)
		})
	}
}

This works fine when a kill is sent directly to the parent process. But in an interactive shell, pressing Ctrl+C sends a SIGINT to the process group (both parent and child). When this happens, the child ends up receiving SIGINT twice: one from the shell (via Ctrl+C) and one relayed from the parent. This is probably a minor issue but could be unexpected for cleanup logic to execute twice.

Additionally, this approach prevents graceful exists on Windows. Since Windows doesn't have signals, Node.js listens for Ctrl+C on stdin so it can be caught. But when a SIGINT (programmatic kill) is received, it terminates the process immediately regardless of a handler:

Sending SIGINT, SIGTERM, and SIGKILL will cause the unconditional termination of the target process, and afterwards, subprocess will report that the process was terminated by signal.

-Process#Signal events | Node.js

FWIW this is how ts-node seems to handle it.

2. Ignore kill signals on parent process

import { constants } from 'os'

const noop = () => {}
const uncatchableSignals = ['SIGKILL', 'SIGSTOP']

for (const signal of Object.keys(constants.signals)) {
	if (!uncatchableSignals.includes(signal)) {
		// Let shell send signals to child directly
		process.on(signal, noop)
	}
}

To prevent the signal from being received twice, I figured maybe we can ignore signals on the parent and let the shell send them to the child directly (e.g. on Ctrl+C).

However, this disables programmatic kills from being handled by the parent so spawning tsx as a child process will not be gracefully killable.

3. Detached child process

import { spawn } from 'child_process'

spawn(process.execPath, process.argv.slice(1), {
	stdio: 'inherit',
	detached: true
})

On Linux/Mac, detaching the child process while inherting the stdio allowed Ctrl+C to only be sent to the parent process (likely because they were no longer in the same process group).

This was convenient because the parent can relay the kill signal to the child and it wouldn't be sent again by the shell. However, on Windows, detaching the child seemed to prevent stdio from getting inherited (Node.js bug? Windows limitation?). None of the child's stdout would be visible.

4. Only relay kill signal if Ctrl+C is not pressed

By detecting Ctrl+C (specifically the keypress), we would be able to distinguish kill signals from interactive shells vs programmatic ones, and only relay signals to the child if it's programmatic.

However, detecting Ctrl+C requires stdin to be in raw mode, which disables normal stdin behavior. It also seems to prevent Ctrl+C from being sent to the child (the child still receives key strokes, but it's not \x03).

This approach doesn't seem possible.


I investigated how npm and pnpm handle this, and seems they both do a simple relay on SIGINT and SIGTERM:

npm: https://github.com/npm/run-script/blob/8e08311358a9f7c361e191b728eaada53eba607b/lib/signal-manager.js#L11
pnpm: https://github.com/pnpm/npm-lifecycle/blob/93939127f6a68e81b00a5758f445f4130c02b4e7/index.js#L293-L295

As stated in Approach 1, this approach has the drawback of the child process receiving the signal twice when triggered via shell shortcut (e.g. Ctrl+C), and it terminates the process immediately on Windows. However, with npm, it seems to inadvertently prevent that by spawning a shell. So, pressing Ctrl+C on Windows prompts "Terminate batch job (Y/N)?", allowing the child to run the SIGINT handler.

Since this approach seems widely adopted, I'm going to move forward with it despite it's limitations.

@gz-95
Copy link
Author

gz-95 commented Oct 13, 2022

@privatenumber thanks for spending time looking into it. Given the circumstances, I think it's reasonable for the child process to handle duplicated signals by not initiating another shutdown procedure if one is already in progress.

Thanks again for your hard work!

@privatenumber
Copy link
Owner

I actually managed to prevent double signal receiving and also preserve Windows support for Ctrl + C.

Will release shortly.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working outdated pr welcome
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants