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

ESM flag breaks NodeJS child process fork #1812

Closed
devversion opened this issue Jun 23, 2022 · 6 comments · Fixed by #1814
Closed

ESM flag breaks NodeJS child process fork #1812

devversion opened this issue Jun 23, 2022 · 6 comments · Fixed by #1814
Milestone

Comments

@devversion
Copy link
Contributor

Search Terms

child process, fork, esm, loader, brotli, config, exec argv, exec path

Expected Behavior

Similar to when running with node --loader ts-node/esm or plain node, the main process should be able to fork and execute other NodeJS scripts (not necessarily another TS file).

When vanilla Node executes, the execArgv never contains the script entry-point, so that the fork can set the new script that is intended to be run.

Actual Behavior

Using the builtin NodeJS fork method does not work when running with ts-node --esm because the process.execPath and process.execArgv contain the brotli-base64 encoded config and the custom loader script. This is resulting in the same script being executed all the time, going into an infinite loop based on the script.

Steps to reproduce the problem

  1. Create a TS ESM file e.g. .mts
  2. Use NodeJS fork and try to load another file, e.g. a plain worker.mjs file
  3. Observe that the worker script is never executing, but instead the orignal .mts file is executed again, and so on.

Minimal reproduction

  1. Clone https://github.com/devversion/ts-node-esm-fork/tree/main
  2. Run yarn test

Specifications

ts-node v10.8.1
node v16.14.2
compiler v4.7.4
Done in 0.35s.
  • Linux 64bit
@cspotcode
Copy link
Collaborator

cspotcode commented Jun 23, 2022 via email

@devversion
Copy link
Contributor Author

Yeah, I haven't looked a lot into the implementation of the child process instantiation and how the bootstrap state works, but it seems like it could also be possible to split up the state into two parts. i.e. the entry-point <-> and the rest of the state. Then the execArgv could be tweaked manually (as it looks like it will be modified anyway) to omit the entry-point part.

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 23, 2022 via email

@devversion
Copy link
Contributor Author

Yes, that is correct. The problem is that the entry-point is encoded into the execArgv as part of the brotli string.

Main script
Exec path /usr/local/google/home/pgschwendtner/bin/node/bin/node
Exec argv [
  '--require',
  '/usr/local/google/home/pgschwendtner/projects/ts-node-fork-esm/node_modules/ts-node/dist/child/child-require.js',
  '--loader',
  'file:///usr/local/google/home/pgschwendtner/projects/ts-node-fork-esm/node_modules/ts-node/child-loader.mjs',
  '/usr/local/google/home/pgschwendtner/projects/ts-node-fork-esm/node_modules/ts-node/dist/child/child-entrypoint.js',
  '--brotli-base64-config=GxYDIBwFbqzNKwWBK0WRZy2ytwVT0y3l67cmCDeJJuiN9+0BimbR6eo7ddM9pd+taU8NXOdtWhRIF0c6JjBlThcFHaaSxHx3OhV1qJfqeamOrnSXQKghsDYp31UUYFUp2F8O4aObUTXirzDqFc9Bzfh1e2t7d7OoDzS5/Wg71x/34jptFGgxT9Fgj+0kYz3tfhZgaxTu4d3EEH6pxnz0+MgXjiTO/wOJK941tQqAO/CUuoiBiWHoVx44QHLTbyaMu5OFe1BNbR1CT+w7RTZA0wFdr5+C10FjzA7xbBJGFLN0hEMJY9D5zx3Yy1pNn2toXSFzJopFHhAHog6M8dM+K6yddvmyzddyZwJfB0Br4zpmYqP1zg8='
]

The brotli config is the following decompressed:

{
    "shouldUseChildProcess": true,
    "isInChildProcess": false,
    "entrypoint": "HOME/projects/ts-node-fork-esm/node_modules/ts-node/dist/bin.js",
    "parseArgvResult": {
        "argv": [
            "HOME/bin/node/bin/node",
            "HOME/projects/ts-node-fork-esm/node_modules/.bin/ts-node",
            "--esm",
            "--project",
            "tsconfig.json",
            "index.mts"
        ],
        "restArgs": [
            "index.mts"
        ],
        "help": false,
        "version": 0,
        "argsRequire": [],
        "print": false,
        "interactive": false,
        "project": "tsconfig.json",
        "esm": true
    },
    "phase2Result": {
        "executeEval": false,
        "executeEntrypoint": true,
        "executeRepl": false,
        "executeStdin": false,
        "cwd": "HOME/projects/ts-node-fork-esm",
        "scriptPath": "HOME/projects/ts-node-fork-esm/index.mts"
    }
}

@cspotcode
Copy link
Collaborator

cspotcode commented Jun 23, 2022

I see, makes sense. As you say, the solution is to pass the entrypoint as a positional argument outside of the brotli payload, so that fork() and related workflows can overwrite it, and ts-node's bootstrapping logic will accept the overwritten entrypoint and execute it.

We will definitely accept a PR with this fix. I'm available either here or on Discord to offer guidance to whomever wants to tackle this. Our tests are pretty good around this stuff, so I wouldn't be afraid to propose a solution via draft pull request.

devversion added a commit to devversion/ts-node that referenced this issue Jun 24, 2022
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`).

Fixes TypeStrong#1812.
@devversion
Copy link
Contributor Author

@cspotcode I attempted to fix this. When you have a chance, please have a look at #1814.

devversion added a commit to devversion/ts-node that referenced this issue Jun 24, 2022
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`).

Fixes TypeStrong#1812.
devversion added a commit to devversion/ts-node that referenced this issue Jul 3, 2022
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`).

Fixes TypeStrong#1812.
@cspotcode cspotcode added this to the 10.9.0 milestone Jul 10, 2022
devversion added a commit to devversion/ts-node that referenced this issue Jul 13, 2022
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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 13, 2022
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.
cspotcode added a commit that referenced this issue Jul 13, 2022
* Fix ESM node processes being unable to fork into other scripts

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`).

Fixes #1812.

* Fix `--cwd` to actually set the working directory and work with ESM child process

Currently the `--esm` option does not necessarily do what the
documentation suggests. i.e. the script does not run as if the working
directory is the specified directory.

This commit fixes this, so that the option is useful for TSConfig
resolution, as well as for controlling the script working directory.

Also fixes that the CWD encoded in the bootstrap brotli state for the
ESM child process messes with the entry-point resolution, if e.g. the
entry-point in `child_process.fork` is relative to a specified `cwd`.

* changes based on review

* lint-fix

* enable transpileOnly in new tests for performance

* Tweak basic working dir tests to verify that --cwd affects entrypoint resolution but not process.cwd()

* update forking tests:

disable non --esm test with comment about known bug and link to tickets
make tests set cwd for fork() call, to be sure it is respected and not overridden by --cwd

* use swc compiler to avoid issue with ancient TS versions not understanding import.meta.url syntax

* Remove tests that I think are redundant (but I've asked for confirmation in code review)

* fix another issue with old TS

* final review updates

Co-authored-by: Andrew Bradley <cspotcode@gmail.com>
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
…caching/reduced complexity

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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
…caching/reduced complexity

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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
…caching/reduced complexity

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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
…caching/reduced complexity

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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
…caching/reduced complexity

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.
devversion added a commit to devversion/ts-node that referenced this issue Jul 14, 2022
…caching/reduced complexity

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.
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Jul 18, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [ts-node](https://typestrong.org/ts-node) ([source](https://github.com/TypeStrong/ts-node)) | devDependencies | minor | [`10.8.2` -> `10.9.1`](https://renovatebot.com/diffs/npm/ts-node/10.8.2/10.9.1) |

---

### Release Notes

<details>
<summary>TypeStrong/ts-node</summary>

### [`v10.9.1`](https://github.com/TypeStrong/ts-node/releases/tag/v10.9.1)

[Compare Source](TypeStrong/ts-node@v10.9.0...v10.9.1)

**Fixed**

-   Workaround nodejs bug introduced in 18.6.0 ([#&#8203;1838](TypeStrong/ts-node#1838)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   Only affects projects on node >=18.6.0 using `--esm`
    -   Older versions of node and projects without `--esm` are unaffected

https://github.com/TypeStrong/ts-node/milestone/18?closed=1

### [`v10.9.0`](https://github.com/TypeStrong/ts-node/releases/tag/v10.9.0)

[Compare Source](TypeStrong/ts-node@v10.8.2...v10.9.0)

**Added**

-   `--project` accepts path to a directory containing a `tsconfig.json` ([#&#8203;1829](TypeStrong/ts-node#1829), [#&#8203;1830](TypeStrong/ts-node#1830)) [@&#8203;cspotcode](https://github.com/cspotcode)
    -   previously it required an explicit filename
-   Added helpful error message when swc version is too old to support our configuration ([#&#8203;1802](TypeStrong/ts-node#1802)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Added `experimentalTsImportSpecifiers` option which allows using voluntary `.ts` file extensions in import specifiers (undocumented except for [API docs](https://typestrong.org/ts-node/api/interfaces/CreateOptions.html#experimentalTsImportSpecifiers)) ([#&#8203;1815](TypeStrong/ts-node#1815)) [@&#8203;cspotcode](https://github.com/cspotcode)

**Fixed**

-   Fixed bug where `child_process.fork()` would erroneously execute the parent's entrypoint script, not the intended child script ([#&#8203;1812](TypeStrong/ts-node#1812), [#&#8203;1814](TypeStrong/ts-node#1814)) [@&#8203;devversion](https://github.com/devversion)
-   Fixed support for jsx modes `"react-jsx"` and `"react-jsxdev"` in swc transpiler ([#&#8203;1800](TypeStrong/ts-node#1800), [#&#8203;1802](TypeStrong/ts-node#1802)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Fixed support for import assertions in swc transpiler ([#&#8203;1817](TypeStrong/ts-node#1817), [#&#8203;1802](TypeStrong/ts-node#1802)) [@&#8203;cspotcode](https://github.com/cspotcode)
-   Fixed bug where calling `repl.evalCode()` with code not ending in a newline would not update the typechecker accordingly ([#&#8203;1764](TypeStrong/ts-node#1764), [#&#8203;1824](TypeStrong/ts-node#1824)) [@&#8203;cspotcode](https://github.com/cspotcode)

https://github.com/TypeStrong/ts-node/milestone/16?closed=1

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xMTEuMSIsInVwZGF0ZWRJblZlciI6IjMyLjExMi4wIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1461
Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants