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

Adopt utility process in terminal/pty host #175335

Closed
bpasero opened this issue Feb 24, 2023 · 14 comments · Fixed by #182631
Closed

Adopt utility process in terminal/pty host #175335

bpasero opened this issue Feb 24, 2023 · 14 comments · Fixed by #182631
Assignees
Labels
insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment shared-process VS Code shared process issues terminal Integrated terminal issues
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Feb 24, 2023

Follow up from #154050

We now have sufficient infrastructure in place to easily fork a UtilityProcess with support of message port communication to other processes:

export class UtilityProcess extends Disposable {

Currently the PTY agent is still running as child process in the shared process housing all terminals:

const ptyHostService = new PtyHostService({
graceTime: LocalReconnectConstants.GraceTime,
shortGraceTime: LocalReconnectConstants.ShortGraceTime,
scrollback: configurationService.getValue<number>(TerminalSettingId.PersistentSessionScrollback) ?? 100
},
localize('ptyHost', "Pty Host"),
configurationService,
environmentService,
logService,
loggerService
);
ptyHostService.initialize();
services.set(ILocalPtyService, this._register(ptyHostService));

We should consider moving the PTY agent out into a standalone utility process to reduce pressure on the shared process (and actually vice versa, reduce pressure on terminals).

@bpasero bpasero added terminal Integrated terminal issues shared-process VS Code shared process issues sandbox Running VSCode in a node-free environment labels Feb 24, 2023
@Tyriar Tyriar added this to the March 2023 milestone Mar 2, 2023
@bpasero
Copy link
Member Author

bpasero commented Mar 2, 2023

Here are some pointers how it works today with the shared process which I think is very similar to the PTY-Host, because:

  • there is only 1 shared process (there is only 1 PTY-Host)
  • every window connects to the shared process (and PTY-Host)
  • to avoid startup perf issues, we want the process to spawn only on demand after editors have restored, triggered from the window [1]
  • shared process and PTY-Host are leveraging bootstrap-fork to come alive as AMD module

1.) electron-sandbox: Workbench Shared process service

Any client in the workbench window that needs a channel to use a service from the shared process does so via the SharedProcessService. This service will:

  • await the LifecyclePhase.Restored phase (here)
  • use the acquirePort() utility method to ask the main process for a MessagePort exchange
  • create a MessagePortClient with the result

The utility method acquirePort does all the heavy lifting of passing a MessagePort over from the main process through the preload script and into the sandboxed window. It expects the vscode:createSharedProcessMessageChannel to be listened on from the main process via Electron IPC mechanics. The main process will respond to the vscode:createSharedProcessMessageChannelResult. The sandbox blog post covers this in more detail.

👉 For the PTY-Host I would pretty much take the workbench shared process service code as is and rename it to PTY specifics. I am not sure if the service has to be so generic to return IChannel. This is more meant for a process that exposes many things, but if the PTY Host only exposes fixed set of methods, I would probably only expose those.
👉 If there is sufficient overlap, we can extract a common base class and reuse the code

2.) electron-main: Shared Process Starter

The SharedProcess in electron-main is the listener to the vscode:createSharedProcessMessageChannel Electron IPC request from any window and is responsible for spawning the process and exchanging message ports. This class will:

  • spawn the UtilityProcess upon first workbench window connection with some payload and the AMD entryPoint
  • calls UtilityProcess.connect() to obtain a MessagePort and send that back to the requesting window via the vscode:createSharedProcessMessageChannelResult Electron IPC channel

👉 you can ignore the workerXY methods, they are for when the deprecated hidden window is used
👉 For the PTY-Host I would again think that a lot of the code can be reused, so we can think about a common super type here

3.) node: Shared Process Main

The SharedProcessMain is the actual module that runs as utility process. This module will:

  • listen to the payload to come in to instantiate itself
  • send some lifecycle events back to the main process via postMessage to signal ready state
  • create a IPCServer to accept MessagePort connections leveraging ipc.mp.ts helper
  • exposes services as IChannel to the clients

👉 I think for the PTY-Host, you already do most of this, the only new thing is to check for the PTY-Host running from a utility process or a bare node.js process and then instantiating the right IPCServer and using the right methods for message communications. We provide a IPCServer for node.js as well via ipc.cp.ts

[1] Possible solution to that is covered in: #175959

@joaomoreno joaomoreno changed the title Consider to move terminals out of shared process Move terminals out of shared process Mar 5, 2023
@joaomoreno joaomoreno changed the title Move terminals out of shared process Adopt utility process in terminal/pty host Mar 5, 2023
@Tyriar Tyriar modified the milestones: March 2023, April 2023 Mar 20, 2023
@Tyriar
Copy link
Member

Tyriar commented Mar 30, 2023

How it currently works:

image

@Tyriar
Copy link
Member

Tyriar commented Mar 30, 2023

Current thinking:

image

@Tyriar
Copy link
Member

Tyriar commented Mar 30, 2023

Source: pty-host-20230330_2.tldr.txt

@Tyriar
Copy link
Member

Tyriar commented Mar 30, 2023

One of the major differences between pty host and the shared process is that the pty host much exist in both the desktop client and the server, whereas the shared process is rolled in to the server process in remote.

@bpasero
Copy link
Member Author

bpasero commented Mar 31, 2023

Actually the file watcher is similar: it exists both in desktop and on the server. On desktop we use a utility process to fork it from the main process and on the server we use a traditional node.js fork, but the code is the same in the end that gets to run. There is an if-else check to see if we run from utility process or node:

let server: ChildProcessServer<string> | UtilityProcessServer;
if (isUtilityProcess(process)) {
server = new UtilityProcessServer();
} else {
server = new ChildProcessServer('watcher');
}
const service = new UniversalWatcher();
server.registerChannel('watcher', ProxyChannel.fromService(service));

@Tyriar
Copy link
Member

Tyriar commented Mar 31, 2023

Having some problems atm while trying to move the pty host service to the main process, the process it creates (still a regular node proc) fails to import node-pty. Not sure why yet

@Tyriar
Copy link
Member

Tyriar commented Mar 31, 2023

It does import node-pty, and it works fine if I break where the WindowsPtyAgent is instantiated the step through the remaining lines in WindowsTerminal.ctor. But if I don't do that, the heartbeats stop and I never get any data from node-pty 😕

@Tyriar
Copy link
Member

Tyriar commented Mar 31, 2023

Something that does happen immediately that is split up when stepping through is seamless relaunch due to extensions activating.

@Tyriar
Copy link
Member

Tyriar commented Mar 31, 2023

Disabling seamless relaunch doesn't fix the problem

@Tyriar
Copy link
Member

Tyriar commented Mar 31, 2023

Everything works fine if I disable conpty

@bpasero bpasero removed this from the April 2023 milestone Apr 25, 2023
@bpasero bpasero added this to the May 2023 milestone Apr 25, 2023
@vscodenpa vscodenpa added the unreleased Patch has not yet been released in VS Code Insiders label May 16, 2023
Tyriar added a commit that referenced this issue May 16, 2023
@vscodenpa vscodenpa added insiders-released Patch has been released in VS Code Insiders and removed unreleased Patch has not yet been released in VS Code Insiders labels May 17, 2023
@bpasero
Copy link
Member Author

bpasero commented Jun 9, 2023

Curious, did you end up having a MessagePort per terminal or is the ptyHost multiplexing?

@Tyriar
Copy link
Member

Tyriar commented Jun 9, 2023

I didn't end up trying the message port per terminal. There's a single message port per window, established here: https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/terminal/electron-sandbox/localTerminalBackend.ts#L83

@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
insiders-released Patch has been released in VS Code Insiders sandbox Running VSCode in a node-free environment shared-process VS Code shared process issues terminal Integrated terminal issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants