Skip to content

Commit

Permalink
Fix ESM node processes being unable to fork into other scripts
Browse files Browse the repository at this point in the history
Currently, Node processes instantiated through the `--esm` flag result
in a child process being created so that the ESM loader can be
registered. This works fine and is reasonable.

The child process approach to register ESM hooks currently prevents
the NodeJS `fork` method from being used because the `execArgv`
propagated into forked processes causes `ts-node` (which is also
propagated as child exec script -- this is good because it allows nested
type resolution to work) to always execute the original entry-point,
causing potential infinite loops because the designated fork module
script is not executed as expected.

This commit fixes this by not encoding the entry-point information into
the state that is captured as part of the `execArgv`. Instead the
entry-point information is always retrieved from the parsed rest command
line arguments in the final stage (`phase4`).

Additionally, this PR streamlines the boostrap mechanism to always
call into the child script, resulting in reduced complexity, and also
improved caching for user-initiated forked processes. i.e. the tsconfig
resolution is not repeated multiple-times because forked processes
are expected to preserve the existing ts-node project. More details
can be found here TypeStrong#1831.

Fixes TypeStrong#1812.
  • Loading branch information
devversion committed Jul 14, 2022
1 parent 0e0da59 commit f609518
Show file tree
Hide file tree
Showing 33 changed files with 428 additions and 217 deletions.
231 changes: 151 additions & 80 deletions src/bin.ts

Large diffs are not rendered by default.

19 changes: 3 additions & 16 deletions src/child/child-entrypoint.ts
@@ -1,24 +1,11 @@
import { BootstrapState, bootstrap } from '../bin';
import { completeBootstrap, BootstrapStateForChild } from '../bin';
import { argPrefix, compress, decompress } from './argv-payload';

const base64ConfigArg = process.argv[2];
if (!base64ConfigArg.startsWith(argPrefix)) throw new Error('unexpected argv');
const base64Payload = base64ConfigArg.slice(argPrefix.length);
const state = decompress(base64Payload) as BootstrapState;
const state = decompress(base64Payload) as BootstrapStateForChild;

state.isInChildProcess = true;
state.tsNodeScript = __filename;
state.parseArgvResult.argv = process.argv;
state.parseArgvResult.restArgs = process.argv.slice(3);

// Modify and re-compress the payload delivered to subsequent child processes.
// This logic may be refactored into bin.ts by https://github.com/TypeStrong/ts-node/issues/1831
if (state.isCli) {
const stateForChildren: BootstrapState = {
...state,
isCli: false,
};
state.parseArgvResult.argv[2] = `${argPrefix}${compress(stateForChildren)}`;
}

bootstrap(state);
completeBootstrap(state);
41 changes: 41 additions & 0 deletions src/child/child-exec-args.ts
@@ -0,0 +1,41 @@
import { pathToFileURL } from 'url';
import { brotliCompressSync } from 'zlib';
import type { BootstrapStateForChild } from '../bin';
import { versionGteLt } from '../util';

const argPrefix = '--brotli-base64-config=';

export function getChildProcessArguments(
enableEsmLoader: boolean,
state: BootstrapStateForChild
) {
if (enableEsmLoader && !versionGteLt(process.versions.node, '12.17.0')) {
throw new Error(
'`ts-node-esm` and `ts-node --esm` require node version 12.17.0 or newer.'
);
}

const nodeExecArgs = [];

if (enableEsmLoader) {
nodeExecArgs.push(
'--require',
require.resolve('./child-require.js'),
'--loader',
// Node on Windows doesn't like `c:\` absolute paths here; must be `file:///c:/`
pathToFileURL(require.resolve('../../child-loader.mjs')).toString()
);
}

const childScriptArgs = [
`${argPrefix}${brotliCompressSync(
Buffer.from(JSON.stringify(state), 'utf8')
).toString('base64')}`,
];

return {
nodeExecArgs,
childScriptArgs,
childScriptPath: require.resolve('./child-entrypoint.js'),
};
}
43 changes: 43 additions & 0 deletions src/child/spawn-child-with-esm.ts
@@ -0,0 +1,43 @@
import { fork } from 'child_process';
import type { BootstrapStateForChild } from '../bin';
import { getChildProcessArguments } from './child-exec-args';

/**
* @internal
* @param state Bootstrap state to be transferred into the child process.
* @param enableEsmLoader Whether to enable the ESM loader or not. This option may
* be removed in the future when `--esm` is no longer a choice.
* @param targetCwd Working directory to be preserved when transitioning to
* the child process.
*/
export function callInChildWithEsm(
state: BootstrapStateForChild,
targetCwd: string
) {
const { childScriptArgs, childScriptPath, nodeExecArgs } =
getChildProcessArguments(/* enableEsmLoader */ true, state);

childScriptArgs.push(...state.parseArgvResult.restArgs);

const child = fork(childScriptPath, childScriptArgs, {
stdio: 'inherit',
execArgv: [...process.execArgv, ...nodeExecArgs],
cwd: targetCwd,
});
child.on('error', (error) => {
console.error(error);
process.exit(1);
});
child.on('exit', (code) => {
child.removeAllListeners();
process.off('SIGINT', sendSignalToChild);
process.off('SIGTERM', sendSignalToChild);
process.exitCode = code === null ? 1 : code;
});
// Ignore sigint and sigterm in parent; pass them to child
process.on('SIGINT', sendSignalToChild);
process.on('SIGTERM', sendSignalToChild);
function sendSignalToChild(signal: string) {
process.kill(child.pid, signal);
}
}
52 changes: 0 additions & 52 deletions src/child/spawn-child.ts

This file was deleted.

52 changes: 37 additions & 15 deletions src/test/esm-loader.spec.ts
Expand Up @@ -362,10 +362,8 @@ test.suite('esm', (test) => {
test.suite('esm child process working directory', (test) => {
test('should have the correct working directory in the user entry-point', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm/ index.ts`,
{
cwd: resolve(TEST_DIR, 'working-dir'),
}
`${BIN_PATH} --esm index.ts`,
{ cwd: './working-dir/esm/' }
);

expect(err).toBe(null);
Expand All @@ -377,33 +375,57 @@ test.suite('esm', (test) => {
test.suite('esm child process and forking', (test) => {
test('should be able to fork vanilla NodeJS script', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm-child-process/ ./process-forking-js/index.ts`
`${BIN_PATH} --esm index.ts`,
{ cwd: './esm-child-process/process-forking-js-worker/' }
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});

test('should be able to fork TypeScript script', async () => {
test('should be able to fork into a nested TypeScript ESM script', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm-child-process/ ./process-forking-ts/index.ts`
`${BIN_PATH} --esm index.ts`,
{ cwd: './esm-child-process/process-forking-nested-esm/' }
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});

test('should be able to fork TypeScript script by absolute path', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm-child-process/ ./process-forking-ts-abs/index.ts`
);
test(
'should be possible to fork into a nested TypeScript script with respect to ' +
'the working directory',
async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm index.ts`,
{ cwd: './esm-child-process/process-forking-nested-relative/' }
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});
expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
}
);

test.suite(
'with NodeNext TypeScript resolution and `.mts` extension',
(test) => {
test.runIf(tsSupportsStableNodeNextNode16);

test('should be able to fork into a nested TypeScript ESM script', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm ./esm-child-process/process-forking-nested-esm-node-next/index.mts`
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});
}
);
});

test.suite('parent passes signals to child', (test) => {
Expand Down
4 changes: 4 additions & 0 deletions src/test/helpers.ts
Expand Up @@ -33,6 +33,10 @@ export const BIN_SCRIPT_PATH = join(
TEST_DIR,
'node_modules/.bin/ts-node-script'
);
export const CHILD_ENTRY_POINT_SCRIPT = join(
TEST_DIR,
'node_modules/ts-node/dist/child/child-entrypoint.js'
);
export const BIN_CWD_PATH = join(TEST_DIR, 'node_modules/.bin/ts-node-cwd');
export const BIN_ESM_PATH = join(TEST_DIR, 'node_modules/.bin/ts-node-esm');

Expand Down
60 changes: 39 additions & 21 deletions src/test/index.spec.ts
Expand Up @@ -5,6 +5,7 @@ import { tmpdir } from 'os';
import semver = require('semver');
import {
BIN_PATH_JS,
CHILD_ENTRY_POINT_SCRIPT,
CMD_TS_NODE_WITH_PROJECT_TRANSPILE_ONLY_FLAG,
nodeSupportsEsmHooks,
nodeSupportsSpawningChildProcess,
Expand Down Expand Up @@ -619,30 +620,27 @@ test.suite('ts-node', (test) => {

test('should have the correct working directory in the user entry-point', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --cwd ./cjs index.ts`,
{
cwd: resolve(TEST_DIR, 'working-dir'),
}
`${BIN_PATH} --cwd ./working-dir/cjs/ index.ts`
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing');
expect(stderr).toBe('');
});

// Disabled due to bug:
// --cwd is passed to forked children when not using --esm, erroneously affects their entrypoint resolution.
// tracked/fixed by either https://github.com/TypeStrong/ts-node/issues/1834
// or https://github.com/TypeStrong/ts-node/issues/1831
test.skip('should be able to fork into a nested TypeScript script with a modified working directory', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --cwd ./working-dir/forking/ index.ts`
);
test(
'should be able to fork into a nested TypeScript script with a modified ' +
'working directory',
async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --cwd ./working-dir/forking/ index.ts`
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});
expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
}
);

test.suite('should read ts-node options from tsconfig.json', (test) => {
const BIN_EXEC = `"${BIN_PATH}" --project tsconfig-options/tsconfig.json`;
Expand Down Expand Up @@ -1132,7 +1130,23 @@ test('Falls back to transpileOnly when ts compiler returns emitSkipped', async (

test.suite('node environment', (test) => {
test.suite('Sets argv and execArgv correctly in forked processes', (test) => {
forkTest(`node --no-warnings ${BIN_PATH_JS}`, BIN_PATH_JS, '--no-warnings');
forkTest(`node --no-warnings ${BIN_PATH_JS}`, BIN_PATH_JS, [
'--no-warnings',
]);

forkTest(
`node --no-warnings ${BIN_PATH_JS} --esm`,
CHILD_ENTRY_POINT_SCRIPT,
[
'--no-warnings',
// Forked child processes should directly receive the necessary ESM loader
// Node flags through `execArgv`, avoiding doubled child process spawns.
'--require',
expect.stringMatching(/child-require.js$/),
'--loader',
expect.stringMatching(/child-loader.mjs$/),
]
);
forkTest(
`${BIN_PATH}`,
process.platform === 'win32' ? BIN_PATH_JS : BIN_PATH
Expand All @@ -1141,7 +1155,7 @@ test.suite('node environment', (test) => {
function forkTest(
command: string,
expectParentArgv0: string,
nodeFlag?: string
nodeFlags?: (string | ReturnType<typeof expect.stringMatching>)[]
) {
test(command, async (t) => {
const { err, stderr, stdout } = await exec(
Expand All @@ -1151,16 +1165,20 @@ test.suite('node environment', (test) => {
expect(stderr).toBe('');
const generations = stdout.split('\n');
const expectation = {
execArgv: [nodeFlag, BIN_PATH_JS, '--skipIgnore'].filter((v) => v),
execArgv: [
...(nodeFlags || []),
CHILD_ENTRY_POINT_SCRIPT,
expect.stringMatching(/^--brotli-base64-config=.*/),
],
argv: [
// Note: argv[0] is *always* BIN_PATH_JS in child & grandchild
// Note: argv[0] is *always* CHILD_ENTRY_POINT_SCRIPT in child & grandchild
expectParentArgv0,
resolve(TEST_DIR, 'recursive-fork/index.ts'),
'argv2',
],
};
expect(JSON.parse(generations[0])).toMatchObject(expectation);
expectation.argv[0] = BIN_PATH_JS;
expectation.argv[0] = CHILD_ENTRY_POINT_SCRIPT;
expect(JSON.parse(generations[1])).toMatchObject(expectation);
expect(JSON.parse(generations[2])).toMatchObject(expectation);
});
Expand Down
@@ -1,13 +1,9 @@
import { fork } from 'child_process';
import { dirname } from 'path';
import { fileURLToPath } from 'url';

// Initially set the exit code to non-zero. We only set it to `0` when the
// worker process finishes properly with the expected stdout message.
process.exitCode = 1;

process.chdir(dirname(fileURLToPath(import.meta.url)));

const workerProcess = fork('./worker.js', [], {
stdio: 'pipe',
});
Expand Down
@@ -1,8 +1,5 @@
{
"compilerOptions": {
"module": "ESNext"
},
"ts-node": {
"swc": true
}
}

0 comments on commit f609518

Please sign in to comment.