node-pty

node-pty copied to clipboard

Environment details

• OS: Arch Linux
• OS version: Linux 5.6.7-zen1-1-zen
• node-pty version: 0.9.0
• electron version: 8.2.3

Issue description

I'm trying to use node-pty in my electron app. But if I try yo import it using:

let pty;
try {
  pty = require('node-pty');
} catch (outerError) {
  console.error('outerError', outerError);
}

I get the following error:

webpack-internal:///ou8/:33 outerError Error: Cannot open /.../node_modules/node-pty/build/Release/pty.node: Error: Loading non-context-aware native module in renderer: '/.../node_modules/node-pty/build/Release/pty.node', but app.allowRendererProcessReuse is true. See https://github.com/electron/electron/issues/18397.
    at Object.eval (webpack-internal:////2OH:1)
    at eval (webpack-internal:////2OH:2)
    at Object./2OH (npm.node-pty.js:11)
    at __webpack_require__ (runtime.js:854)
    at fn (runtime.js:151)
    at eval (webpack-internal:///NEGr:26)
    at Object.NEGr (npm.node-pty.js:37)
    at __webpack_require__ (runtime.js:854)
    at fn (runtime.js:151)
    at eval (webpack-internal:///ubDT:13)

I know I could just set app.allowRendererProcessReuse = false;, but in consideration of newer electron versions I don't want to do this.

@develerik Currently parts of node-pty are not thread-safe (like ptsname), thus it should never be used from concurrent workers. We cannot simply switch to NAN_MODULE_WORKER_ENABLED before resorting to thread-safe internals (we basically have thread safety issues on the C end, not the JS end).

@Tyriar Since nodejs has matured its official worker support, it seems we have to rewrite parts in a thread-safe fashion. This also might cover parts of #375 (windows conpty handles).

Yes this is on our backlog to do as we upgrade electron in vscode

As I just went through this with a native addon, here are some remarks:

Context aware aims for the ability to load a native module into different JS contexts (multiple JS engines). The contexts itself may live in the same or a different thread (a worker would always be a new thread with its own context). Therefore a module load/unload should not show any side effects on the module state of other contexts. This is pretty easy with modules, that only do direct C function mapping and do not interfere with the JS context beside arguments and return values (reentrant & threadsafe regarding JS side). Those modules work automatically by calling NAN_MODULE_WORKER_ENABLED instead of NODE_MODULE for nodejs >= 10.

Regarding this node-pty is well decoupled from the JS context (no static global data). On C side the only vulnerable calls seem to be ptsname (see this file for replacement/shims) and prolly the libuv interaction (Is uv_default_loop threadsafe?). Imho special care is needed at the fork-exec-waitpid logic and the pty_context carried around as pty_baton: https://github.com/microsoft/node-pty/blob/bcdc826f365dd014403992d41b286e815798459a/src/unix/pty.cc#L84-L91

The stored JS callback there is likely to segfault, if not properly cleared during a JS context shutdown (AddEnvironmentCleanupHook). Also it should never be accessed from a "wrong thread/ JS context". If libuv loop access is not thread-safe, imho the whole waitpid-thread handling is on trial, not clue yet how to rework this in a thread-safe manner.

Another option nodejs offers to get context aware / multithread support is to use the N-API interface. Not sure if this is worth a shot yet for stability and performance reasons. It certainly would lower the maintenance burden in the long run.

Another option nodejs offers to get context aware / multithread support is to use the N-API interface. Not sure if this is worth a shot yet for stability and performance reasons. It certainly would lower the maintenance burden in the long run.

This is the hope as then we don't need to recompile it for every different node version, and hopefully that electron node version error people get will go away (if they're building it wrong).

This is the hope as then we don't need to recompile it for every different node version, and hopefully that electron node version error people get will go away (if they're building it wrong).

According to the documentation, there is no guarantee in ABI stability across Node.js major versions if a native module depends on uv.h APIs like uv_default_loop, uv_async_t and uv_thread_t.

btw its on windows mac and linux

Any news? (And is there any way I can help?)

@Tyriar this becomes quite an urgent issue now as node-pty is now unusable in Electron 14+ because of the deprecation of non context aware modules see https://github.com/electron/electron/issues/18397

@gpetrov A few users (me among them) have been working on a port to Rust (napi-rs) that is context-aware. https://github.com/corwin-of-amber/node-pty (this is my fork, that currently includes macOS and Linux support).

Windows is not supported atm. Help is appreciated! You can try the macOS/Linux version with

npm i npmin/node-pty#0.10.0

btw Visual Studio Code is built on Electron and does use node-pty. Does that mean they are using an older version of Electron? I believe it is a priority for them too.

(EDIT: I checked and indeed my VSC Insiders is using Electron 11.3.0)

What a shame that Microsoft is stuck back in time with Electron 11 which is already end of life!

So no security updates for them...

VS Code Insiders is on Electron 13.5.2, important security updates are backported as needed since updating Electron for such a complex app is a lot of work.

You can see issues we're tracking for the Electron 16 update at https://github.com/microsoft/vscode/issues?q=is%3Aopen+is%3Aissue+label%3Aelectron-16-update+

Oh right! Seems like I have not updated my Insiders in a while :-)

Any news on this?

@daniel-brenot and I have ported node-pty to Rust with NAPI, and the latest working version for Unix is here: https://github.com/corwin-of-amber/node-pty/tree/rust-port

I got stuck on Windows because I am a bit clueless w.r.t. Windows API. In particular, this PR needs some help.

@corwin-of-amber That my friend is brilliant! Works like a charm. My app is only targeting Mac and Linux so at least for me this is a complete solution. I've never used rust before (and i guess i havent used it now), but it really just worked, I was pleasantly surprised. Thank you!

Oh and just FYI if it helps I built it on an M1.

Any progress here @Tyriar ?

Electron 18 is just released and the Electron team stopped with all security fixes under Electron 15

So we are hitting a dead end here.

@gpetrov the Windows version is what currently blocks it. I am so clueless with Windows API, it is probably just a silly thing. If you know anyone with some Windows experience who can take a look at the PR I mentioned above, that might push it forward.

@gpetrov VS Code is on Electron 17 now and node-pty is still working fine there

Thanks @Tyriar that is great news! Just tried it and indeed seems to be working fine with Electron 17.

Maybe it is due to the NAPI support then the native modules are considered context aware.

Is the compatibility with the latest node-pty betas only or it is all fine?

I see vscode is using beta 11

VS Code had some trouble adopting the latest version and I put that work on hold to focus on some other things for the time being.

@gpetrov NAPI modules are context aware (at least NAPI v3). But I see that the main branch of microsoft/node-pty is still using nan. @Tyriar did I miss any recent development? Where is the version of node-pty that uses NAPI instead?

@corwin-of-amber I don't think so?

FYI, vscode loads node-pty in a process that has only single v8 context for the lifetime of the application, hence this is not an issue when moving forward with newer electron versions.

FWIW I'd also recommend all consumers of node-pty in Electron do this as well, otherwise the data coming from the pty is bound to node event loop which is busy with other things and it ends up slowing everything down considerably. I don't 100% understand everything going on here but the basics are running node-pty on a separate dedicated process and sending the data over from there ends up being much faster.

@Tyriar @deepak1556 Sounds like the logic for spawning a separate v8 process, running node-pty there, and sending input/output streams would make an excellent complementary package, if that logic can be extracted from vscode.

But what about if you have multiple node-pty processes running? Should we do a fork of node-pty for each process? So you are basically doubling up the number of external processes...

@deepak1556 how did vscode bypassed the check of electron for prohibiting running non context aware modules based on nan? Is it by running it with node only fork or as renderer fork?

Sounds like the logic for spawning a separate v8 process, running node-pty there, and sending input/output streams would make an excellent complementary package, if that logic can be extracted from vscode.

@corwin-of-amber it's too tightly coupled to VS Code, it includes it's unique IPC mechanism and supports many other features beyond what node-pty is meant to do like reconnection, "properties", etc.

@gpetrov VS Code has a single "pty host" process that hosts all node-pty processes, currently it communicates via:

renderer process <-> shared process (a bare electron window used by multiple windows) <-> pty host

The pty host proc is launched with ELECTRON_RUN_AS_NODE, not sure if any other flags are needed.

Any progress, or any alternative/fork that work with electron 16?

Yes. The branch I'm working on should solve that problem along with numerous other issues. It replaces the current implementation of nodepty with rust and NAPI. There's still troubleshooting happening on that project though so for the time being I'm not sure when that work will be completed.