Vscode: Create a node-pty host process with flow control and event batching

[Tyriar]

As of around a year ago node-pty is launched directly from the renderer process, while this does mean ctrl+c is responsive it has several downsides:

• node-pty crashes will take down the entire window (https://github.com/microsoft/vscode/issues/71966, https://github.com/microsoft/vscode/issues/71789, https://github.com/microsoft/vscode/issues/74181)
• Data events from node-pty seems to be blocked by the event loop, I suspect this is main the reason conpty is slow, it may also be slowing down non-Windows data flow too
• The remote case the connection can get overwhelmed by fast producing programs.
• node-pty can freeze the window when a busy loop is run in the terminal https://github.com/microsoft/vscode/issues/106391

My proposal for these problems:

• Reintroduce terminalProcess to host node-pty, calling it nodePtyHostProcess is probably a better name, this protects the extension host and renderer from crashes
• Batch data events inside nodePtyHostProcess (https://github.com/microsoft/vscode/issues/54093), reducing the amount of total events in favor of larger ones
• Introduce a flow control mechanism which ensures the pty does not get too far ahead of xterm.js. This is a little more tricky for the remote case, one idea is to send an ack every x bytes from the renderer and nodePtyHostProcess will pause if it hasn't received the yth last ack (x and y need to be experimented with and may depend on latency) (related https://github.com/xtermjs/xterm.js/pull/2122, https://github.com/microsoft/node-pty/pull/304)

[reinux]

Is this also keeping extensions from talking to terminal processes via stdin? Because F# Interactive with Ionide has this severe issue right now where everything, including input sent from the extension, has to be echoed.

That also means that since user code is displayed in the terminal, they can't put #line directives in it, which means exception stack traces have incorrect line numbers.

Compounded with the fact that the terminal is already slow, sending 300+ lines of code takes a really long time.

[Tyriar]

@reinux I don't think so, I'll comment there.

[Tyriar]

Commit that moved the process into renderer: https://github.com/microsoft/vscode/commit/35cee02c9fa9a109c4d75c0f686793f903864ae5

[Tyriar]

Commit that converted to use amd https://github.com/microsoft/vscode/commit/ed9cdf55b0c9e327ab634cc3e09c97b63671af03

[Tyriar]

Did some exploration into this.

How terminal processes are launched currently

                           |
                           v
                 TerminalProcessManager
                    |              |
                    v              v
TerminalProcessExtHostProxy    TerminalProcess
             |                 (renderer proc)
             v
   (an ext host connection)
             |
             v
      TerminalProcess
      (ext host proc)

Issues:

• In proc node-pty can crash the renderer when conpty/winpty misbehave
• Slow node event loop means slower data flow from the pty, I believe this is the primary reason terminals are a lot slower on Windows

Using just the ext host

                 |
                 v
        TerminalProcessManager
                 |
                 v
    TerminalProcessExtHostProxy
                 |
                 v
(local or remote ext host connection)
                 |
                 v
          TerminalProcess

Issues:

• In proc node-pty can crash the ext host when conpty/winpty misbehave
• More data from terminal can flood ext host connection, eg. after running yes command (flow control is eventual solution to this)
• Blocker: SSH extension depends on terminals which means the local ext host would have to be up and running

Prototype: https://github.com/microsoft/vscode/pull/81106

Moving the processes out of proc, this is how it used to be (before ext host processes)

               |
         (forked process)
               v
        TerminalProcess
        (uses node-pty)

Issues:

• Slower data flow as we're passing around JS strings
• Can flood connection (note this is not an issue locally because the nodejs event loop blocks it)

Proposed solution

                           |
                           v            
                 TerminalProcessManager 
                    |              |    
                    v              v    
TerminalProcessExtHostProxy    TerminalPty <--> TerminalInstanceService[2]
             |                                             |
             v                                       (forked proc[1])
   (an ext host connection)                                v
             |                                   TerminalPtyHostProcess
             |
             v                     
        TerminalPty <--> ExtHostTerminalTerminalService[2]
                                       |
                                 (forked proc[1])
                                       v
                             TerminalPtyHostProcess

[1] Uses ext host socket-based message protocol
[2] The terminal services own the host process and establish the connection

• Crash protection for ext host and renderer, if node-pty crashes it would take down only TerminalPtyHostProcess (and all terminals it's running)
• Local terminals are still available for SSH extension
• Data flow
  
  
  
  • Faster data flow as messages passed over socket are binary data which node-pty can now emit and xterm.js can consume
  
  
  • Add flow control which pauses node-pty sockets when they are getting too ahead of xterm.js parsing of the data
  
  
  • TerminalPtyHostProcess is basically just handling data flow for all terminals, so it shouldn't block the pty as much
  
  
  • Can also batch data to reduce the amount of messages coming
  
  

Prototype: https://github.com/microsoft/vscode/pull/81105

Note that this could be split into 2 stages:

1. Moving into new process, communicate via ChildProcess.send and add flow control
2. Adding new protocol and enabling fast transfer of data via ArrayBuffers

On flow control

We recently exposed a callback on xterm's write which fires when the data is parsed. This will make flow control much easier that the earlier proposals I talked about. For example we could provide a callback for event nth write and send messages over to TerminalPtyHostProcess to indicate where the renderer is at in parsing the data.

[Tyriar]

I did an exploration into moving node-pty into the main process (https://github.com/microsoft/vscode/pull/99897) and it worked pretty well. It's just a prototype and falls over when it comes to pty lifecycle management but it seems like it would be quite easy to tweak it to restore terminals across reloads https://github.com/microsoft/vscode/issues/20013 if it lived in the main process (as the prototype shows). Another benefit is it speeds up the terminal output _significantly_, tree running in the same directly was several times faster due to event loop being freed up, I expect this to also happen when we get https://github.com/microsoft/node-pty/pull/415 in, even when node-pty is hosted in the renderer.

My plan is to eventually move node-pty into a process that's forked from the main process and then establishing a direct socket connection for the terminal if possible to the window so we keep pretty much everything of the main process except for the setting up of the connection.

The branch in action with node-pty hosted on the main process and (the hacky) reload persistence:

[tmandry]

Restoring the actual terminal session would be great, but restoring the shell layout with current directory and scrollback saved would give 99% of the benefits for me.