-
-
Notifications
You must be signed in to change notification settings - Fork 535
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
Fix ESM node processes being unable to fork into other scripts #1814
Changes from 4 commits
796b593
d78e437
38befcc
ad19d9f
e112f7c
112a47c
d52c0b5
b7bcdcf
3d9e040
5164e2f
58ddc33
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,8 @@ export function main( | |
const state: BootstrapState = { | ||
shouldUseChildProcess: false, | ||
isInChildProcess: false, | ||
entrypoint: __filename, | ||
isCli: true, | ||
tsNodeScript: __filename, | ||
parseArgvResult: args, | ||
}; | ||
return bootstrap(state); | ||
|
@@ -62,7 +63,12 @@ export function main( | |
export interface BootstrapState { | ||
isInChildProcess: boolean; | ||
shouldUseChildProcess: boolean; | ||
entrypoint: string; | ||
/** | ||
* True if bootstrapping the ts-node CLI process or the direct child necessitated by `--esm`. | ||
* false if bootstrapping a subsequently `fork()`ed child. | ||
*/ | ||
isCli: boolean; | ||
tsNodeScript: string; | ||
parseArgvResult: ReturnType<typeof parseArgv>; | ||
phase2Result?: ReturnType<typeof phase2>; | ||
phase3Result?: ReturnType<typeof phase3>; | ||
|
@@ -73,12 +79,16 @@ export function bootstrap(state: BootstrapState) { | |
if (!state.phase2Result) { | ||
state.phase2Result = phase2(state); | ||
if (state.shouldUseChildProcess && !state.isInChildProcess) { | ||
// Note: When transitioning into the child-process after `phase2`, | ||
// the updated working directory needs to be preserved. | ||
return callInChild(state); | ||
} | ||
} | ||
if (!state.phase3Result) { | ||
state.phase3Result = phase3(state); | ||
if (state.shouldUseChildProcess && !state.isInChildProcess) { | ||
// Note: When transitioning into the child-process after `phase2`, | ||
// the updated working directory needs to be preserved. | ||
return callInChild(state); | ||
} | ||
} | ||
|
@@ -264,8 +274,7 @@ function parseArgv(argv: string[], entrypointArgs: Record<string, any>) { | |
} | ||
|
||
function phase2(payload: BootstrapState) { | ||
const { help, version, code, interactive, cwdArg, restArgs, esm } = | ||
payload.parseArgvResult; | ||
const { help, version, cwdArg, esm } = payload.parseArgvResult; | ||
|
||
if (help) { | ||
console.log(` | ||
|
@@ -319,28 +328,14 @@ Options: | |
process.exit(0); | ||
} | ||
|
||
// Figure out which we are executing: piped stdin, --eval, REPL, and/or entrypoint | ||
// This is complicated because node's behavior is complicated | ||
// `node -e code -i ./script.js` ignores -e | ||
const executeEval = code != null && !(interactive && restArgs.length); | ||
const executeEntrypoint = !executeEval && restArgs.length > 0; | ||
const executeRepl = | ||
!executeEntrypoint && | ||
(interactive || (process.stdin.isTTY && !executeEval)); | ||
const executeStdin = !executeEval && !executeRepl && !executeEntrypoint; | ||
|
||
const cwd = cwdArg || process.cwd(); | ||
/** Unresolved. May point to a symlink, not realpath. May be missing file extension */ | ||
const scriptPath = executeEntrypoint ? resolve(cwd, restArgs[0]) : undefined; | ||
const cwd = cwdArg ? resolve(cwdArg) : process.cwd(); | ||
|
||
// If ESM is explicitly enabled through the flag, stage3 should be run in a child process | ||
// with the ESM loaders configured. | ||
if (esm) payload.shouldUseChildProcess = true; | ||
|
||
return { | ||
executeEval, | ||
executeEntrypoint, | ||
executeRepl, | ||
executeStdin, | ||
cwd, | ||
scriptPath, | ||
}; | ||
} | ||
|
||
|
@@ -372,7 +367,15 @@ function phase3(payload: BootstrapState) { | |
esm, | ||
experimentalSpecifierResolution, | ||
} = payload.parseArgvResult; | ||
const { cwd, scriptPath } = payload.phase2Result!; | ||
const { cwd } = payload.phase2Result!; | ||
|
||
// NOTE: When we transition to a child process for ESM, the entry-point script determined | ||
// here might not be the one used later in `phase4`. This can happen when we execute the | ||
// original entry-point but then the process forks itself using e.g. `child_process.fork`. | ||
// We will always use the original TS project in forked processes anyway, so it is | ||
// expected and acceptable to retrieve the entry-point information here in `phase2`. | ||
// See: https://github.com/TypeStrong/ts-node/issues/1812. | ||
const { entryPointPath } = getEntryPointInfo(payload); | ||
|
||
const preloadedConfig = findAndReadConfig({ | ||
cwd, | ||
|
@@ -387,7 +390,12 @@ function phase3(payload: BootstrapState) { | |
compilerHost, | ||
ignore, | ||
logError, | ||
projectSearchDir: getProjectSearchDir(cwd, scriptMode, cwdMode, scriptPath), | ||
projectSearchDir: getProjectSearchDir( | ||
cwd, | ||
scriptMode, | ||
cwdMode, | ||
entryPointPath | ||
), | ||
project, | ||
skipProject, | ||
skipIgnore, | ||
|
@@ -403,23 +411,77 @@ function phase3(payload: BootstrapState) { | |
experimentalSpecifierResolution as ExperimentalSpecifierResolution, | ||
}); | ||
|
||
// If ESM is enabled through the parsed tsconfig, stage4 should be run in a child | ||
// process with the ESM loaders configured. | ||
if (preloadedConfig.options.esm) payload.shouldUseChildProcess = true; | ||
|
||
return { preloadedConfig }; | ||
} | ||
|
||
/** | ||
* Determines the entry-point information from the argv and phase2 result. This | ||
* method will be invoked in two places: | ||
* | ||
* 1. In phase 3 to be able to find a project from the potential entry-point script. | ||
* 2. In phase 4 to determine the actual entry-point script. | ||
* | ||
* Note that we need to explicitly re-resolve the entry-point information in the final | ||
* stage because the previous stage information could be modified when the bootstrap | ||
* invocation transitioned into a child process for ESM. | ||
* | ||
* Stages before (phase 4) can and will be cached by the child process through the Brotli | ||
* configuration and entry-point information is only reliable in the final phase. More | ||
* details can be found in here: https://github.com/TypeStrong/ts-node/issues/1812. | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for adding all these comments. It makes sense to me that we must repeat resolution in phase 4. Are there any situations where we know, without any risk, that we can skip this duplicate resolution? It would be a performance optimization. If we cannot safely do this, then we shouldn't. But if there's a safe way to do this, then I might try my hand at adding it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we could re-use this when the This feels like a micro optimization through, not worth it personally as this is just some simple logical expressions. |
||
function getEntryPointInfo(state: BootstrapState) { | ||
const { code, interactive, restArgs } = state.parseArgvResult!; | ||
const { cwd } = state.phase2Result!; | ||
const { isCli } = state; | ||
|
||
// Figure out which we are executing: piped stdin, --eval, REPL, and/or entrypoint | ||
// This is complicated because node's behavior is complicated | ||
// `node -e code -i ./script.js` ignores -e | ||
const executeEval = code != null && !(interactive && restArgs.length); | ||
const executeEntrypoint = !executeEval && restArgs.length > 0; | ||
const executeRepl = | ||
!executeEntrypoint && | ||
(interactive || (process.stdin.isTTY && !executeEval)); | ||
const executeStdin = !executeEval && !executeRepl && !executeEntrypoint; | ||
|
||
/** | ||
* Unresolved. May point to a symlink, not realpath. May be missing file extension | ||
* NOTE: resolution relative to cwd option (not `process.cwd()`) is legacy backwards-compat; should be changed in next major: https://github.com/TypeStrong/ts-node/issues/1834 | ||
*/ | ||
const entryPointPath = executeEntrypoint | ||
? isCli | ||
? resolve(cwd, restArgs[0]) | ||
: resolve(restArgs[0]) | ||
: undefined; | ||
|
||
return { | ||
executeEval, | ||
executeEntrypoint, | ||
executeRepl, | ||
executeStdin, | ||
entryPointPath, | ||
}; | ||
} | ||
|
||
function phase4(payload: BootstrapState) { | ||
const { isInChildProcess, entrypoint } = payload; | ||
const { isInChildProcess, tsNodeScript } = payload; | ||
const { version, showConfig, restArgs, code, print, argv } = | ||
payload.parseArgvResult; | ||
const { cwd } = payload.phase2Result!; | ||
const { preloadedConfig } = payload.phase3Result!; | ||
|
||
const { | ||
entryPointPath, | ||
executeEntrypoint, | ||
executeEval, | ||
cwd, | ||
executeStdin, | ||
executeRepl, | ||
executeEntrypoint, | ||
scriptPath, | ||
} = payload.phase2Result!; | ||
const { preloadedConfig } = payload.phase3Result!; | ||
executeStdin, | ||
} = getEntryPointInfo(payload); | ||
|
||
/** | ||
* <repl>, [stdin], and [eval] are all essentially virtual files that do not exist on disc and are backed by a REPL | ||
* service to handle eval-ing of code. | ||
|
@@ -566,12 +628,13 @@ function phase4(payload: BootstrapState) { | |
|
||
// Prepend `ts-node` arguments to CLI for child processes. | ||
process.execArgv.push( | ||
entrypoint, | ||
tsNodeScript, | ||
...argv.slice(2, argv.length - restArgs.length) | ||
); | ||
// TODO this comes from BoostrapState | ||
|
||
// TODO this comes from BootstrapState | ||
process.argv = [process.argv[1]] | ||
.concat(executeEntrypoint ? ([scriptPath] as string[]) : []) | ||
.concat(executeEntrypoint ? ([entryPointPath] as string[]) : []) | ||
.concat(restArgs.slice(executeEntrypoint ? 1 : 0)); | ||
devversion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// Execute the main contents (either eval, script or piped). | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
import { brotliCompressSync, brotliDecompressSync, constants } from 'zlib'; | ||
|
||
/** @internal */ | ||
export const argPrefix = '--brotli-base64-config='; | ||
|
||
/** @internal */ | ||
export function compress(object: any) { | ||
return brotliCompressSync(Buffer.from(JSON.stringify(object), 'utf8'), { | ||
[constants.BROTLI_PARAM_QUALITY]: constants.BROTLI_MIN_QUALITY, | ||
}).toString('base64'); | ||
} | ||
|
||
/** @internal */ | ||
export function decompress(str: string) { | ||
return JSON.parse( | ||
brotliDecompressSync(Buffer.from(str, 'base64')).toString() | ||
); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,16 +1,24 @@ | ||
import { BootstrapState, bootstrap } from '../bin'; | ||
import { brotliDecompressSync } from 'zlib'; | ||
import { argPrefix, compress, decompress } from './argv-payload'; | ||
|
||
const base64ConfigArg = process.argv[2]; | ||
const argPrefix = '--brotli-base64-config='; | ||
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; | ||
payload.parseArgvResult.restArgs = process.argv.slice(3); | ||
const state = decompress(base64Payload) as BootstrapState; | ||
|
||
bootstrap(payload); | ||
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); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,7 @@ import { | |
TEST_DIR, | ||
tsSupportsImportAssertions, | ||
tsSupportsResolveJsonModule, | ||
tsSupportsStableNodeNextNode16, | ||
} from './helpers'; | ||
import { createExec, createSpawn, ExecReturn } from './exec-helpers'; | ||
import { join, resolve } from 'path'; | ||
|
@@ -358,6 +359,88 @@ 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 ./working-dir/esm/ index.ts` | ||
); | ||
|
||
expect(err).toBe(null); | ||
expect(stdout.trim()).toBe('Passing'); | ||
expect(stderr).toBe(''); | ||
}); | ||
|
||
test.suite( | ||
'with NodeNext TypeScript resolution and `.mts` extension', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likely yes. Although these tests are still a little different:
|
||
(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 --cwd ./working-dir/esm-node-next/ index.ts` | ||
); | ||
|
||
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 --cwd ./esm-child-process/process-forking/ index.ts` | ||
); | ||
|
||
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 --cwd ./esm-child-process/process-forking-nested-esm/ index.ts` | ||
); | ||
|
||
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 --cwd ./esm-child-process/process-forking-nested-relative/ index.ts` | ||
); | ||
|
||
expect(err).toBe(null); | ||
expect(stdout.trim()).toBe('Passing: from main'); | ||
expect(stderr).toBe(''); | ||
} | ||
); | ||
|
||
test.suite( | ||
'with NodeNext TypeScript resolution and `.mts` extension', | ||
devversion marked this conversation as resolved.
Show resolved
Hide resolved
|
||
(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(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this renamed to indicate that this is the path to ts-node's bootstrapper, but not the user's desired entrypoint?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I renamed that mostly to avoid the ambiguity here (at least ambiguity to me). Happy to revert if you would want it the old way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renaming is good, makes sense to me.