Host conout socket in a worker

[Tyriar]

Conpty can send a final screen update after ClosePseudoConsole which waits for the socket
to be drained before continuing. When both the socket draining and closing occurs on the
same thread a deadlock can occur which locks up VS Code's UI, the only ways to fix are
to close the client or kill the misbehaving conhost instance. This change moves the
handling of the conout named pipe to a Worker which bumps the minimum Node version
requirement to 12 (10 with a flag). This change may also have positive performance
benefits as called out in microsoft/vscode#74620

The worker change also happens for winpty, it seems to work there too.

Fixes #375

[jerch]

@Tyriar Does this work with high input data pressure from the slave side? I wonder if it needs some flow control measures to be passed along to avoid turning the worker thread into a memory hog.

[Tyriar]

This change may also have positive performance benefits as called out in microsoft/vscode#74620

I've validated this, tree in a large directory is many times faster when it's running in VS Code's extension host (more process hops), compared to before when running in the browser window 🎉

Does this work with high input data pressure from the slave side? I wonder if it needs some flow control measures to be passed along to avoid turning the worker thread into a memory hog.

I don't think flow control is needed as a result of this change because the socket is piped, things are responsive with tree which dumps a lot of data. Since it's dealing with named pipes all the way through which block correctly I think it just works.

---

When trying to validate in VS Code I ran into something I've not yet figured out how to solve; worker_threads is disallowed in Electron windows and I can't find a way to enable them. While we're moving towards a place where node-pty isn't hosted in the Electron window so we can enable the sandbox, it's not in that place just yet. It's fairly easy to move it to the extension host but then it delays launching until the extension host is up and running 😢

The Electron thing seems like a problem since then Hyper and other Electron-based terminals will need big changes to get it to work, and we can't use web workers unless nodeIntegrationInWorker is enabled which is generally bad practice.

[Tyriar]

Also validated that this does indeed fix the hang using this repro microsoft/vscode#71966 (comment)

[jerch]

@Tyriar About electron and worker threads - would it be possible to resort here to an iframe hack? Imho those would run in a separate JS context, but surely the merging back of the data streams will be an issue with that. Idk - is there a specific reason, why plain JS workers are prohibited in electron?

[Tyriar]

@jerch we're moving quickly towards having no node integration in the browser window and using an iframe would be going in the opposite direction. node-pty will probably eventually live in another process off of the electron main process (regular node), that solution should work in the long run, at least for VS Code.

is there a specific reason, why plain JS workers are prohibited in electron?

Trying to figure that out, this is the exception that's thrown:

[jerch]

Maybe this helps in the meantime?

let win = new BrowserWindow({
  webPreferences: {
    nodeIntegrationInWorker: true
  }
})

Taken from https://www.electronjs.org/docs/tutorial/multithreading.

[Tyriar]

@jerch that's just for web workers though. Support both node workers and web workers is one option but it's super ugly, web workers wouldn't be typed and we'd need to loosen security restrictions of the app.

[jerch]

I see, well seems getting the separation done on pure node level is the only option then.

[Tyriar]

Yeah, looking into how difficult it would be to do it the right way.

[Tyriar]

@deepak1556 let me know that node workers should be working in Electron 8 (and if they don't it's an unknown bug).

[Tyriar]

Ok It's not working on Electron 8 either ☹️

electron/electron#18540 (comment)

[jerch]

@Tyriar Normal webworkers are funtional in electron render stage? Would it be possible to move the custom logic from a node worker to a normal web worker here? Not sure if webworkers have access to nodejs module stuff though. Quite rough edges still I guess...

[Tyriar]

@jerch they would work currently but we would need to enable node integration in them which would be loosening the security restrictions of vscode that we're trying to tighten up, plus it would be super hacky code that is difficult to test without publishing the npm package. I think waiting on Electron to fix it to bring it in is probably the way to go.

[Tyriar]

It was closed as wontfix because the renderer will only allow web workers electron/electron#18540 (comment). I guess the way forward is to move the node-pty process out of the renderer process and hope that we get a more direct channel of communication between an arbitrary node process and an electron renderer to avoid the data hopping across multiple processes.

[jerch]

@Tyriar This move sounds reasonable to not mix the different JS contexts further up (which surely would create lots of tension and bad hacks on low level, a nightmare from a security/isolation perspective).

On a sidenote - is it even worth getting this PR properly working, given that you want to reshape node-pty with the newer nodejs API soon? Imho that step would end up as an almost full rewrite, since the APIs are very different. Just wondering...

[Tyriar]

given that you want to reshape node-pty with the newer nodejs API soon?

Do you mean the non-context aware issue #405? The direction with this PR is moving away from using Electron renderers all together which is best practice from a security perspective so I don't think we need to do that anymore.

[gpetrov]

Can this be merged and released as 0.10.0-beta17 maybe @Tyriar
those freezes are really annoying

[Tyriar]

@gpetrov unfortunately not, this won't work in Electron because of electron/electron#18540 (comment)

The options are to optionally use a web worker as well which would be a mess to maintain or wait until node-pty moves out of the renderer process in VS Code which is a bit of an involved change microsoft/vscode#74620.

[gpetrov]

@Tyriar No problem - I installed your hang_fix branch directly from github and it is running perfectly. No more freezes. We are running on NWJS with the latest Chrome 85 engine so have all the latest node workers support.

[Tyriar]

@gpetrov glad to hear it's working out 👍, we'll get either this or a similar fix in for this eventually.

[Tyriar]

FYI this is available to pick up in [email protected], I'll be bringing it into VS Code soon

[Eugeny]

@Tyriar just FYI, it appears that worker_threads isn't available in Electron renderer processes.

[jerch]

@Eugeny Shouldn't this just be Worker, as it is within the chrome engine? Or is worker access generally blocked in renderer ctx in electron?

[Eugeny]

@jerch web workers would work (sorry) but this PR explicitly imports Node worker_threads

[jerch]

@Eugeny
I see. Hmm, guess this boils down to the question if normal web workers would also allow loading nodejs modules to some extend? I am not familiar with the electron env, all I get is that the main renderer process got altered in a way to allow certain nodejs modules to be loaded. With workers in mind this leads to a convoluted situation:

• base process - all nodejs, ok
  • nodejs worker_threads - threads derived from base process, all nodejs semantics, ok
• chrome main process - all DOM JS semantics + some nodejs modules (if allowed)
  • web worker within browser engine - derived from chrome main process - Can they load nodejs modules as well?

To me it seems logically not to allow worker_threads in the browser context at all, as they are a spinoff of the main nodejs context, which has nothing to do with the browser-ish part of electron. The weird hack around electron is the ability to load nodejs modules in the main browser context, so did they extend that to spinoffs of the browser context (web workers) as well? If they carry forward the hack to browser spinoffs as well, you could just use nodejs modules in web workers. (Tbh I would not allow that, it smells too much like state sync hell.)

Maybe this needs some clarification from an electron dev, to explain the thread/worker bounderies more clearly.

[Eugeny]

They allow Node modules in browser contexts (both in renderer and worker, controlled separately via nodeIntegration and nodeIntegrationInWorker), however worker_threads specifically seem to be broken / not compiled in. Creating a new Worker throws The V8 platform used by this instance of Node does not support creating Workers. Looks like the Electron team prefers that we stick to Web Workers in the renderer process instead.

Does VSCode host node-pty in its main process? I'm currently loading node-pty into the renderer processes to save on IPC. Not sure about other library users.

[mofux]

I think it's time to move it to the main process as it looks like nodeIntegration is discouraged by electron because of the security implications it has.

[jerch]

nodeIntegrationInWorker - ah ok, so that is possible, it still gets a 😨 from my side.

@Eugeny Since this is possible - what would need real worker_threads support for in the browser ctx? It is basically equivalent, though with slightly different semantics (module resolution, different global, exposed browser worker primitives the nodejs worker impl does not know about and such).

In general I second @mofux's remark - being able to load native node modules in the browser ctx cries for bad security conditions. Still I understand your hestitation - the additional IPC will have negative perf impact (but thats not that bad if properly prebuffered to reduce event loop pressure, as you can test with our repo demo). The main downside of decoupled node-pty from xterm.js process is the need for proper flow control (which you kinda get for free by running node-pty directly in the renderer due to event loop backpressure).

[sedwards2009]

https://www.electronjs.org/docs/tutorial/multithreading

Native nodejs modules inside browser worker threads is NOT recommended. I was under the impression that the problem was related to dynamic native code loading itself not being thread safe. 🤷‍♂️

I've been using node-pty from the main process (no other threads etc) and sending data to/from the browser process for years. I haven't noticed any big latency problems related to that set up. I do get to side-step all these questions around threading though and that does save a lot of worrying.

[Tyriar]

You can follow the linked issues for more details but the summary is:

• Electron does not enable worker_threads in renderers by design
• This PR was blocked on VS Code moving node-pty to live outside the renderer Move terminal process to main vscode#116185
• Node integration does not only pose security risks, but also proper sandboxing enables Electron renderers to be recycled and launch quicker
• While counterintuitive, performance on Windows should be improved quite a bit by moving node-pty out of the renderer because conpty sends a lot of events and it was bottlenecked by the busy/slow renderer event loop
• Support for direct IPC via message ports between node.js to electron renderer is coming, we aren't keeping node-pty on main but rather moving it to a child of main to protect its cycles and also in case there happens to be a native crash/hang in conpty (which would take down all electron renderers)

Maybe this is a big enough of a change to warrant a bump to v1.0.0, we're essentially following semver already.

[jerch]

While counterintuitive, performance on Windows should be improved quite a bit by moving node-pty out of the renderer because conpty sends a lot of events and it was bottlenecked by the busy/slow renderer event loop

Ah yepp, the browser engines have a hard time to deal with tons of small chunks per function call on the event loop. Seems libuv's loop does not suffer that much under these conditions, which should show a remarkable speed gain for this edge case.

But - can't ConPty send bigger chunk batches in the first place (would also reduce the whole kernel/userland ctx switch pressure by the OS)? 🤔

[Tyriar]

But - can't ConPty send bigger chunk batches in the first place (would also reduce the whole kernel/userland ctx switch pressure by the OS)? 🤔

They probably could, that would be a discussion for the terminal repo. Regardless, conpty updates with windows so we'd need to deal with these smaller chunks anyway.

[Tyriar]

Note that this doesn't play nicely with asar currently: microsoft/vscode#116373 (comment)