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 3333005 commit 4c3390a
Show file tree
Hide file tree
Showing 33 changed files with 390 additions and 204 deletions.
230 changes: 150 additions & 80 deletions src/bin.ts

Large diffs are not rendered by default.

19 changes: 3 additions & 16 deletions src/child/child-entrypoint.ts
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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'),
};
}
48 changes: 20 additions & 28 deletions src/child/spawn-child.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,30 @@
import type { BootstrapState } from '../bin';
import { spawn } from 'child_process';
import { pathToFileURL } from 'url';
import { versionGteLt } from '../util';
import { argPrefix, compress } from './argv-payload';
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 callInChild(state: BootstrapState) {
if (!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 child = spawn(
process.execPath,
[
'--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(),
require.resolve('./child-entrypoint.js'),
`${argPrefix}${compress(state)}`,
...state.parseArgvResult.restArgs,
],
{
stdio: 'inherit',
argv0: process.argv0,
}
);
export function callInChild(
state: BootstrapStateForChild,
enableEsmLoader: boolean,
targetCwd: string
) {
const { childScriptArgs, childScriptPath, nodeExecArgs } =
getChildProcessArguments(enableEsmLoader, 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);
Expand Down
52 changes: 37 additions & 15 deletions src/test/esm-loader.spec.ts
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
53 changes: 23 additions & 30 deletions src/test/index.spec.ts
Original file line number Diff line number Diff line change
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,17 +1130,10 @@ 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(
`${BIN_PATH}`,
process.platform === 'win32' ? BIN_PATH_JS : BIN_PATH
);
forkTest(`node --no-warnings ${BIN_PATH_JS}`, '--no-warnings');
forkTest(`${BIN_PATH}`);

function forkTest(
command: string,
expectParentArgv0: string,
nodeFlag?: string
) {
function forkTest(command: string, nodeFlag?: string) {
test(command, async (t) => {
const { err, stderr, stdout } = await exec(
`${command} --skipIgnore ./recursive-fork/index.ts argv2`
Expand All @@ -1151,16 +1142,18 @@ 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: [
nodeFlag,
CHILD_ENTRY_POINT_SCRIPT,
expect.stringMatching(/^--brotli-base64-config=.*/),
].filter((v) => v),
argv: [
// Note: argv[0] is *always* BIN_PATH_JS in child & grandchild
expectParentArgv0,
CHILD_ENTRY_POINT_SCRIPT,
resolve(TEST_DIR, 'recursive-fork/index.ts'),
'argv2',
],
};
expect(JSON.parse(generations[0])).toMatchObject(expectation);
expectation.argv[0] = BIN_PATH_JS;
expect(JSON.parse(generations[1])).toMatchObject(expectation);
expect(JSON.parse(generations[2])).toMatchObject(expectation);
});
Expand Down
4 changes: 2 additions & 2 deletions src/test/repl/repl-environment.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import { context, expect } from '../testlib';
import * as getStream from 'get-stream';
import {
CHILD_ENTRY_POINT_SCRIPT,
CMD_TS_NODE_WITH_PROJECT_FLAG,
ctxTsNode,
delay,
Expand Down Expand Up @@ -145,8 +146,7 @@ test.suite(
return modulePaths;
}

// Executable is `ts-node` on Posix, `bin.js` on Windows due to Windows shimming limitations (this is determined by package manager)
const tsNodeExe = expect.stringMatching(/\b(ts-node|bin.js)$/);
const tsNodeExe = CHILD_ENTRY_POINT_SCRIPT;

test('stdin', async (t) => {
const { stdout } = await execTester({
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -1,8 +1,5 @@
{
"compilerOptions": {
"module": "ESNext"
},
"ts-node": {
"swc": true
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@ import { fileURLToPath } from 'url';
// worker process finishes properly with the expected stdout message.
process.exitCode = 1;

process.chdir(join(dirname(fileURLToPath(import.meta.url)), 'subfolder'));

const workerProcess = fork('./worker.ts', [], {
const projectDir = dirname(fileURLToPath(import.meta.url));
const workerProcess = fork(join(projectDir, 'worker.mts'), [], {
stdio: 'pipe',
});

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"compilerOptions": {
"module": "NodeNext"
},
"ts-node": {
"transpileOnly": true
}
}
Loading

0 comments on commit 4c3390a

Please sign in to comment.