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 13, 2022
1 parent aa5ec36 commit 7c0678f
Show file tree
Hide file tree
Showing 36 changed files with 638 additions and 129 deletions.
282 changes: 207 additions & 75 deletions src/bin.ts

Large diffs are not rendered by default.

10 changes: 4 additions & 6 deletions src/child/child-entrypoint.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { BootstrapState, bootstrap } from '../bin';
import { completeBootstrap, BootstrapStateForChild } from '../bin';
import { brotliDecompressSync } from 'zlib';

const base64ConfigArg = process.argv[2];
Expand All @@ -7,10 +7,8 @@ if (!base64ConfigArg.startsWith(argPrefix)) throw new Error('unexpected argv');
const base64Payload = base64ConfigArg.slice(argPrefix.length);
const payload = JSON.parse(
brotliDecompressSync(Buffer.from(base64Payload, 'base64')).toString()
) as BootstrapState;
payload.isInChildProcess = true;
payload.entrypoint = __filename;
payload.parseArgvResult.argv = process.argv;
) as BootstrapStateForChild;

payload.parseArgvResult.restArgs = process.argv.slice(3);

bootstrap(payload);
completeBootstrap(payload);
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'),
};
}
57 changes: 25 additions & 32 deletions src/child/spawn-child.ts
Original file line number Diff line number Diff line change
@@ -1,37 +1,30 @@
import type { BootstrapState } from '../bin';
import { spawn } from 'child_process';
import { brotliCompressSync } from 'zlib';
import { pathToFileURL } from 'url';
import { versionGteLt } from '../util';
import { fork } from 'child_process';
import type { BootstrapStateForChild } from '../bin';
import { getChildProcessArguments } from './child-exec-args';

const argPrefix = '--brotli-base64-config=';
/**
* @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: BootstrapStateForChild,
enableEsmLoader: boolean,
targetCwd: string
) {
const { childScriptArgs, childScriptPath, nodeExecArgs } =
getChildProcessArguments(enableEsmLoader, state);

/** @internal */
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}${brotliCompressSync(
Buffer.from(JSON.stringify(state), 'utf8')
).toString('base64')}`,
...state.parseArgvResult.restArgs,
],
{
stdio: 'inherit',
argv0: process.argv0,
}
);
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
88 changes: 88 additions & 0 deletions src/test/esm-loader.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {
TEST_DIR,
tsSupportsImportAssertions,
tsSupportsResolveJsonModule,
tsSupportsStableNodeNextNode16,
} from './helpers';
import { createExec, createSpawn, ExecReturn } from './exec-helpers';
import { join, resolve } from 'path';
Expand Down Expand Up @@ -358,6 +359,93 @@ 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 index.ts`,
{ cwd: './working-dir/esm/' }
);

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

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

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

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

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 index.ts`,
{ cwd: './esm-child-process/process-forking/' }
);

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

test('should be able to fork into a nested TypeScript ESM script', async () => {
const { err, stdout, stderr } = await exec(
`${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 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('');
}
);

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) => {
test.runSerially();

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
48 changes: 34 additions & 14 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 @@ -617,6 +618,30 @@ 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 ./working-dir/cjs/ index.ts`
);

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

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('');
}
);

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 @@ -1105,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 @@ -1124,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
@@ -0,0 +1,23 @@
import { fork } from 'child_process';
import { dirname, join } 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;

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

let stdout = '';

workerProcess.stdout.on('data', (chunk) => (stdout += chunk.toString('utf8')));
workerProcess.on('error', () => (process.exitCode = 1));
workerProcess.on('close', (status, signal) => {
if (status === 0 && signal === null && stdout.trim() === 'Works') {
console.log('Passing: from main');
process.exitCode = 0;
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"compilerOptions": {
"module": "NodeNext"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
const message: string = 'Works';

console.log(message);
20 changes: 20 additions & 0 deletions tests/esm-child-process/process-forking-nested-esm/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import { fork } from 'child_process';

// 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;

const workerProcess = fork('./worker.ts', [], {
stdio: 'pipe',
});

let stdout = '';

workerProcess.stdout.on('data', (chunk) => (stdout += chunk.toString('utf8')));
workerProcess.on('error', () => (process.exitCode = 1));
workerProcess.on('close', (status, signal) => {
if (status === 0 && signal === null && stdout.trim() === 'Works') {
console.log('Passing: from main');
process.exitCode = 0;
}
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
{
"type": "module"
}
Loading

0 comments on commit 7c0678f

Please sign in to comment.