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

Support npm:pm2 #16753

Open
bartlomieju opened this issue Nov 22, 2022 · 18 comments
Open

Support npm:pm2 #16753

bartlomieju opened this issue Nov 22, 2022 · 18 comments
Assignees
Labels
needs investigation requires further investigation before determining if it is an issue or not node API polyfill Related to various "node:*" modules APIs node compat

Comments

@bartlomieju
Copy link
Member

    > @iugo how is it same error? What command are you trying to run?

run command deno run -A npm:pm2 --help.

same error, error info is x.y.z is not a function.

Originally posted by @iugo in #16679 (comment)

@bartlomieju bartlomieju added needs investigation requires further investigation before determining if it is an issue or not node compat labels Nov 22, 2022
@khrj
Copy link
Contributor

khrj commented Nov 22, 2022

Min. example

import chalk from "npm:chalk@3"
console.log(chalk.bold.green("Hello"))

Errors:

error: Uncaught TypeError: chalk.bold.green is not a function
console.log(chalk.bold.green("Hello"))
                       ^

@khrj
Copy link
Contributor

khrj commented Nov 22, 2022

The same issue is also present when using createRequire

image

@khrj
Copy link
Contributor

khrj commented Nov 22, 2022

I've dissected through the code for chalk v3 and have managed to get pure JS (no deps) code that works in node, but fails in Deno. I'm not completely sure yet but it seems like Node is handling object prototypes differently from Deno, I'll try to get a cleaner example and then post it here

@khrj
Copy link
Contributor

khrj commented Nov 22, 2022

Note that the code works in node and not in Deno even when not using npm specifiers / createRequire, so this isn't an issue with the npm compatibility layer (or std/node)

@khrj
Copy link
Contributor

khrj commented Nov 22, 2022

Duplicate of #13321

@bartlomieju
Copy link
Member Author

If this is because of chalk@3 then it's because of missing "proto" that Deno doesn't support and will not in the future. So the best bet is to ask pm2 maintainers to update their dependencies.

@khrj
Copy link
Contributor

khrj commented Nov 22, 2022

Yes, it is because of that -- is there no chance of implementing __proto__ to internally call Object.setPrototypeOf? If not for Deno modules, at least for npm specifiers to not break existing modules.

I've run grep through the node_modules of the top 10 packages on npm and nearly all of them have .__proto__ somewhere.

@iuioiua
Copy link
Contributor

iuioiua commented Dec 6, 2022

FYI, @khrj is aiming to fix this in Unitech/pm2#5489

@khrj
Copy link
Contributor

khrj commented Dec 7, 2022

Note that even after proto is fixed, pm2 will still be broken by #16784

@bartlomieju bartlomieju added the node API polyfill Related to various "node:*" modules APIs label Mar 4, 2024
@lino-levan
Copy link
Contributor

pm2 now works with unsafe proto!
image

@bartlomieju
Copy link
Member Author

This still doesn't work for me. It appears that we're not correctly handling "ipc" option in child_process module in new ChildProcess():

Spawning PM2 daemon with pm2_home=/Users/ib/.pm2
spawning
caught error Error: Not implemented: toDenoStdio pipe=string (ipc)
    at notImplemented (ext:deno_node/_utils.ts:27:9)
    at toDenoStdio (ext:deno_node/internal/child_process.ts:379:5)
    at new ChildProcess (ext:deno_node/internal/child_process.ts:197:16)
    at Object.spawn (node:child_process:192:10)
    at module.exports.Client.launchDaemon (file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:256:40)
    at file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:104:10
    at file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:321:14
    at Array.processTicksAndRejections (ext:deno_node/_next_tick.ts:37:11)
    at eventLoopTick (ext:core/01_core.js:166:29)
error: Uncaught TypeError: Cannot read properties of undefined (reading 'unref')
    at ChildProcess.unref (ext:deno_node/internal/child_process.ts:323:19)
    at module.exports.Client.launchDaemon (file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:274:9)
    at file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:104:10
    at file:///Users/ib/Library/Caches/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:321:14
    at Array.processTicksAndRejections (ext:deno_node/_next_tick.ts:37:11)
    at eventLoopTick (ext:core/01_core.js:166:29)

CC @littledivy is it something you could look at? Since we already support IPC in subprocesses it feels like there's something misconfigured that makes it crash.

@littledivy
Copy link
Member

@bartlomieju I'm not getting the IPC error. It fails on .unref

$ deno run -A --unstable-unsafe-proto npm:pm2 --help
[PM2] Spawning PM2 daemon with pm2_home=/home/divy/.pm2
error: Uncaught TypeError: Cannot read properties of undefined (reading 'unref')
    at ChildProcess.unref (ext:deno_node/internal/child_process.ts:221:19)
    at module.exports.Client.launchDaemon (file:///home/divy/.cache/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:274:9)
    at file:///home/divy/.cache/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:104:10
    at file:///home/divy/.cache/deno/npm/registry.npmjs.org/pm2/5.3.1/lib/Client.js:321:14
    at Array.processTicksAndRejections (ext:deno_node/_next_tick.ts:25:11)
    at eventLoopTick (ext:core/01_core.js:166:29)

@bartlomieju
Copy link
Member Author

@littledivy - correct. I added additional logs to surface that error (the catch handler for when the process is spawned).

@littledivy
Copy link
Member

littledivy commented Mar 22, 2024

Ah, I see the problem now. Our implementation only supports non-stdio IPC.

We need to implement support for stdio as raw fds and "ipc" in Deno.Command.

const process = new Deno.Command(command, {
  stdout: "ipc",
  // or stdout: 3
}).spawn();

@bartlomieju
Copy link
Member Author

Huh, i thought we already had it. Is it any different than the current IPC support we have?

@littledivy
Copy link
Member

This should be straightforward to implement:

  • Add "ipc" (or maybe "ipc_for_internal_use") in Deno.Command's stdin, stdout, stderr. Set the args.ipc FD and the ipc code path should automatically setup the channels.

  • Allow numeric rids in Deno.Command's stdio options that deserialize to StdioOrRid in runtime/ops/process.rs, there is already code to map file rids to stdio in there from Deno.run.

  • Finally, allow "ipc" and numeric values in the toDenoStdio conversion in ext/node/child_process.ts

littledivy added a commit that referenced this issue Jun 7, 2024
…4106)

Add supports for "ipc" and fd options in child_process spawn API.

Internal changes: Adds a hidden rid and "ipc_for_internal_use" option to
Deno.Command. Used by `node:child_process`

Example:
```js
const out = fs.openSync("./logfile.txt", 'a')
const proc = spawn(process.execPath, ["./main.mjs", "child"], {
  stdio: ["ipc", out, "inherit"]
});
```

Ref #16753
nathanwhit pushed a commit that referenced this issue Jun 13, 2024
…4106)

Add supports for "ipc" and fd options in child_process spawn API.

Internal changes: Adds a hidden rid and "ipc_for_internal_use" option to
Deno.Command. Used by `node:child_process`

Example:
```js
const out = fs.openSync("./logfile.txt", 'a')
const proc = spawn(process.execPath, ["./main.mjs", "child"], {
  stdio: ["ipc", out, "inherit"]
});
```

Ref #16753
@littledivy
Copy link
Member

littledivy commented Jul 4, 2024

stdio IPC support landed. A few more things to solve before this works:

By default, pm2 spawns its daemon using node (hard-coded). The
daemon fails to start in non-BYONM context because Node doesn't
understand Deno npm resolution.

In BYONM context the daemon spawns but IPC between Deno and Node
doesn't work because of implementation differences currently.

We have 2 options:

  • Override node <daemon> to deno run <daemon> in child_process#spawn
  • Try to connect with Node IPC

@birkskyum
Copy link
Contributor

It would in general be great to have a way to override nested uses of node with deno, even by default, similar to the '--bun' flag used in that runtime (which will also be default eventually)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs investigation requires further investigation before determining if it is an issue or not node API polyfill Related to various "node:*" modules APIs node compat
Projects
None yet
Development

No branches or pull requests

7 participants