non-context-aware native module

[ghost]

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.

[jerch]

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

[Tyriar]

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

[jerch]

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:

node-pty/src/unix/pty.cc

Lines 84 to 91 in bcdc826

	struct pty_baton {
	Nan::Persistent<v8::Function> cb;
	int exit_code;
	int signal_code;
	pid_t pid;
	uv_async_t async;
	uv_thread_t tid;
	};

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.

[Tyriar]

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

[vadim-termius]

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.

[ShlomiRex]

btw its on windows mac and linux

[corwin-of-amber]

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

[gpetrov]

@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 electron/electron#18397

[corwin-of-amber]

@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

[corwin-of-amber]

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)

[gpetrov]

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

[Tyriar]

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+

[corwin-of-amber]

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

[vrunhofen]

Any news on this?

[corwin-of-amber]

@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.

[vrunhofen]

@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.

[gpetrov]

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.

[corwin-of-amber]

@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.

[Tyriar]

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

[gpetrov]

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

[Tyriar]

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.

[corwin-of-amber]

@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?

[Tyriar]

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

[deepak1556]

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.

[Tyriar]

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.

[corwin-of-amber]

@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.

[gpetrov]

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?

[Tyriar]

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.

[sunjw]

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

[daniel-brenot]

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.

[gpetrov]

@Tyriar this really should be given more priority to fully move to context aware module and NAPI.

Using the VS code way and forking a separate process just for pty is really a very bad can of worms giving all kind of problems.
Although it works, it is very slow because of the many timeouts and process management that needs to be done. As the VS code team mentioned inside their huge amount of patches and timeouts integrated to make it work, see:

https://github.com/microsoft/vscode/blob/226d9ee540d18677c0c18d06fb870df5121197e4/src/vs/platform/terminal/node/terminalProcess.ts#L27

Also killing the pty process or destroy always gives an error:

[stderr] C:\xxxxxx\node_modules\node-pty\lib\conpty_console_list_agent.js:17
var consoleProcessList = getConsoleProcessList(shellPid);
                         ^

Error: AttachConsole failed
    at Object.<anonymous> (C:\xxxxxx\node_modules\node-pty\lib\conpty_console_list_agent.js:17:26)
    at Module._compile (node:internal/modules/cjs/loader:1116:14)
    at Object.Module._extensions..js (node:internal/modules/cjs/loader:1169:10)
    at Module.load (node:internal/modules/cjs/loader:988:32)
    at Module._load (node:internal/modules/cjs/loader:829:12)
    at Function.c._load (node:electron/js2c/asar_bundle:5:13343)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

commands work, but without doing pty.kill or pty.destroy - a ghost conhost.exe stays active for ever and more and more gets spawned and each pty spawn.

[Tyriar]

@gpetrov I think all those timeouts are issues even if it's on the same process. Also a separate process ends up faster not slower as mentioned in #405 (comment), and also provides process isolation so the renderer process isn't taken down just in case there is a native exception.

[gpetrov]

@Tyriar good to know!

Any explanation on the attachConsole error? It happens on pty.kill or pty.destroy and try catch doesn't help.

Also communication with ipc to the separate process is quite challenging as after intensive pty usage the ipc dies and the whole process needs to be restarted. So a lot more process monitoring is needed which is also a great overhead.

[Tyriar]

Any explanation on the attachConsole error? It happens on pty.kill or pty.destroy and try catch doesn't help.

@gpetrov not sure about the AttachConsole thing, I haven't seen it. Maybe the PID being used is wrong or doesn't exist?

node-pty/src/win/conpty_console_list.cc

Line 20 in 1674722

if (!AttachConsole(pid)) {

Currently we don't pass back more info on why it failed so it looks like it could be one of:

If the calling process is already attached to a console, the error code returned is ERROR_ACCESS_DENIED. If the specified process does not have a console, the error code returned is ERROR_INVALID_HANDLE. If the specified process does not exist, the error code returned is ERROR_INVALID_PARAMETER.

via https://docs.microsoft.com/en-us/windows/console/attachconsole#remarks

---

Also communication with ipc to the separate process is quite challenging as after intensive pty usage the ipc dies and the whole process needs to be restarted. So a lot more process monitoring is needed which is also a great overhead.

That problem is called flow control, xterm.js has a guide on how to handle it which is basically a framework to pause and unpause the pty socket based on ack messages from the renderer. Using a separate process should result in much faster data throughput, flow control should ensure the pty doesn't get too far ahead of the renderer so the communication channel doesn't get flooded and the terminal remains responsive (eg. to ctrl+c).

[gpetrov]

@Tyriar thanks for the extended info!

The attachConsole error appear when using single forked process and multiple pty.spawns in it and when killing them.

I thought the point was to have a single process not a separate process per pty right?

But the MS docs say:

A process can be attached to at most one console. If the calling process is already attached to a console, the error code returned is ERROR_ACCESS_DENIED.

Single process with multiple pty spawns in it, works fine and multiple conhost.exe are spawned but just the error appear when they are killed - the kill works though

[Tyriar]

@gpetrov yes a single process for all ptys.

A process can be attached to at most one console. If the calling process is already attached to a console, the error code returned is ERROR_ACCESS_DENIED.

The "process" here is the shell process (cmd.exe, pwsh.exe, etc.) which attaches to a console via that call.

[lonnywong]

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 How to do that? Can you give some code for example? Thanks very much.

[Tyriar]

@lonnywong you can look at VS Code but it's quite the complex example that probably doesn't really translate well to your case. You just need to setup IPC in some way to communicate back and forth with the new process, there are many ways to do this and isn't specific to this library.

[ravicious]

@lonnywong A good example that's less complex than VS Code is Tabby. Take a look at how the IPC events are utilized in both files.

https://github.com/Eugeny/tabby/blob/v1.0.177/app/lib/pty.ts
https://github.com/Eugeny/tabby/blob/v1.0.177/tabby-local/src/session.ts