Avoid sending all terminal data to all renderer processes

[alexdima]

From my understanding of how ILocalPtyService works, it looks to me that all process data reaches all renderer processes due to the IPtyService.onProcessData event. So if there is a spammy terminal in one VS Code window, that output will reach all VS Code windows, and then it is the LocalTerminalService that drops in the VS Code window side the output not meant for one of its terminals (using the ? operators).

I have run into a similar situation where I need to launch extension host processes, but I am only interested in the stdout/stderr/etc. for the process associated to the current window, so I have added something called onDynamic* events to shared process services such that only a specific window can listen to a specific event. You can find an example in IExtensionHostStarter. Any method that begins with onDynamic will be considered a dynamic event and will be propagated correctly to the shared process side. I think the limitation is that a single argument can be passed in, due to how the channel-server listen works.

cc @bpasero

[bpasero]

I would actually think there is more to it: #131798 asks for a way to allow for an isolated communication channel to a window and @deepak1556 made an exploration to use web workers to have multiple isolated MessagePort via workers to a window. Maybe terminals IPC could change in a way that 1 terminal process uses 1 message port for communication so that even a single spammy terminal is not causing a performance issue for anything else that communicates with that window?

[Tyriar]

Kind of sounds like this is a duplicate of #131798, the plan eventually is for the shared process to only establish the connection and then it gets skipped via a direct renderer <-> node channel. We haven't heard much of an issue about this so I'd rather wait and make the proper fix when #131798 is ready.

[bpasero]

@Tyriar yeah I will explore this for the file watcher in October, @deepak1556 explored this and figured out a seamingly working solution that involves web workers with node integration. Example code is at https://gist.github.com/deepak1556/884d729f284880bf6d05302102742b1f

Basic flow:

• shared process creates a worker with node integration enabled
• message ports are connecting the worker with the workbench window
• worker forks process
• communication from child process ends up in worker
• worker sends child process output to renderer via message port

[Tyriar]

@bpasero I think that's still a temporary solution though, we want to avoid as many process jumps as possible even if they're on a different thread.

[bpasero]

@Tyriar I think @deepak1556 should clarify, my understanding was that this is THE solution.

[bpasero]

@Tyriar @meganrogge if you want to play with the shared process worker support for terminals, I just pushed a first version of it and adopted for file watchers.

On a high level:

• there is a new ISharedProcessWorkerWorkbenchService accessible from the workbench
• it provides a createWorkerChannel method to fork a process from a node.js enabled worker in the shared process and returns a IChannel for the communication

On a low level:

• the shared process will create one node.js enabled web worker per kind of process (in other words, 1 worker for all watchers)
• within the web worker a process will be forked per window (this can be changed if terminals need a different model)
• a MessagePort will be created per process used for communication

Alternatively, if this service is not useful for terminals because you have rolled your own process architecture, I would still think that you can do this:

• drop the ptyAgent process
• change it to a node.js enabled web worker
• use the web worker for message port communication (instead of shared process)

The worker is created from vs/platform/sharedProcess/electron-browser/sharedProcessWorkerService and the worker implementation is in vs/platform/sharedProcess/electron-browser/sharedProcessWorkerMain.

[Tyriar]

@bpasero process isolation also prevents crashes in node-pty from taking down the shared process, don't really want to lose that even though I'm not aware of any more that can happen.

[bpasero]

Yeah I understand, that is not something the file watching worker is concerned about because it does nothing else then the message transfer between child process and the message port.

Nevertheless, there could be 1 web worker that forks the PTY agent to avoid that the shared process main thread is busy handling terminal IO.

[bpasero]

So if there is a spammy terminal in one VS Code window, that output will reach all VS Code windows

Btw I am not 100% sure this is true. When a window opens, a pair of MessagePort is created for the window communicating to the shared process. In other words, there is 1 pair of MessagePort per window. I would expect that the data from one window should never end up on the MessagePort of a different window. Would be good to clarify.

[Tyriar]

This will be fixed as part of #175335

[Tyriar]

Fixed!