Enable ConPTY by default in terminals on Windows #44468

gzdunek · 2024-07-19T15:24:48Z

ConPTY is an official Windows API for bridging discrepancies between Unix-style terminals and Windows consoles.
It's better at rendering colors and handling resizing (although this one is still not ideal) than winpty.

Implementation

We detect Windows version in ptyService.ts and based on that decide if ConPTY should be enabled or not. Then we pass that information to two places: node-pty and xterm.js.

There is a chance that ConPTY may not work well in some setups. Because of that it can be disabled using terminal.windowsBackend in the app config.

Changelog: Teleport Connect now uses ConPTY for better terminal resizing and accurate color rendering on Windows, with an option to disable it in the app config

github-actions · 2024-07-19T15:30:09Z

🤖 Vercel preview here: https://docs-8s5hko9h5-goteleport.vercel.app/docs/ver/preview

…ass boolean all the way through

github-actions · 2024-07-23T10:02:32Z

🤖 Vercel preview here: https://docs-flcylr3w1-goteleport.vercel.app/docs/ver/preview

web/packages/teleterm/src/services/pty/ptyService.ts

ravicious · 2024-07-24T11:14:02Z

web/packages/teleterm/src/sharedProcess/ptyHost/ptyProcess.ts

@@ -76,8 +82,7 @@ export class PtyProcess extends EventEmitter implements IPtyProcess {
        // https://unix.stackexchange.com/questions/123858
        cwd: this.options.cwd || getDefaultCwd(this.options.env),
        env: this.options.env,
-        // Turn off ConPTY due to an uncaught exception being thrown when a PTY is closed.
-        useConpty: false,
+        useConpty: this.options.useConpty,


Hmm, every time I open a new shell, there's a new conhost.exe under Task Manager -> Details. If I explicitly close a tab, it goes away. But when I just close Connect, that conhost.exe never goes away.

Screen.Recording.2024-07-24.at.14.43.17.mov

That isn't the case with winpty, where a new conhost.exe is created, but it goes away when closing Connect. conhost.exe spawned by winpty doesn't have those extra arguments.

I'm trying to reproduce it, but with no luck.
In the recording below, conhost.exe processes are killed both when closing tabs one by one and when closing the entire app:

conhostmov.mov

I also don't see these extra arguments.
EDIT: I had that column disabled in the task manager, nvm.

I've just tried your build. conhost.exe goes away when starting a local terminal. But when connecting through SSH to a node, it stays open after closing Connect.

I've tested this only with SSH nodes. Because of the bug with the css prop, I cannot access additional action menu and because the shortcut to open a new terminal tab doesn't work inside the VM, I couldn't open a local terminal. ;f

I can confirm, it happens for SSH connections.
I'm trying to to dispose the pty processes when the shared process exits, but it's quite hard to debug since I can't get any logs from there when the main process kills it :(

I'd look if there are any extra node-pty options there to take care of the issue or if someone else had the same problem. It's interesting that this happens with ConPTY but not winpty.

I didn't find any node-pty options that would take care of that. When it comes to similar issues, I found some (microsoft/node-pty#333, Eugeny/tabby#6318).
The most useful seems this PR from vscode microsoft/vscode#118437.

However, it seems to me that simply killing all pty process on exit would work. I tested this by setting a 30 seconds timeout after the shared process launched; after that, I didn't see any remaining conhost.exe processes.
The problem is that process.on('exit') doesn't seem to work on Windows and our cleanup code is not invoked at all :/ I think we need to send a graceful kill message to the shared process in a different way.

Okay, I'm making progress:

To gracefully kill the shared process we can send a message through the Node.js IPC:

terminateWithTimeout(this.sharedProcess, 5_000, process => process.send('gracefullyKill') )

and then listen for it on the Node.js side:

process.on('message', message => { if (message === 'gracefullyKill') { server.forceShutdown(); ptyHostService.dispose(); process.exit(0); } });

This works fine.
2. It turned out that simply killing all processes and then exiting doesn't work (as I said before). Adding a timeout before process.exit(0) helps, I'm looking now what timeouts vscode uses.

I have it 🥵
To make it work I needed two things:

Gracefully killing the shared process on Windows (done with process.send).

Waiting for the pty processes to exit before closing the app. Killing the pty process takes some time (from my testing, the time between calling ptyProcess.kill and ptyProcess.onExit was around 400 ms). If I'm correct, in vsocde they set timeouts to achieve waiting for processes to exit, but I thought that the better option is to simply wait for onExit to be called (I took an inspiration from terminateWithTimeout).

Tested on Windows with ConPTY enabled/disabled and on macOS. I didn't see any unterminated processes.

Screen.Recording.2024-07-25.at.17.03.15.mov

ravicious

Requesting changes because of the processes left over.

github-actions · 2024-07-25T15:34:01Z

🤖 Vercel preview here: https://docs-ebg5kb4jz-goteleport.vercel.app/docs/ver/preview

ravicious

Looking good. 👍

I'm not sure how I feel about the graceful kill message through IPC, especially since we already have "infrastructure" in place to gracefully terminate tsh on Windows. OTOH, the way we kill tsh on Windows is a huge workaround, so perhaps it's best to stick to IPC which is guaranteed to work.

web/packages/teleterm/src/sharedProcess/ptyHost/ptyHostService.ts

ravicious · 2024-07-26T13:52:23Z

web/packages/teleterm/src/mainProcess/types.ts

@@ -276,3 +276,5 @@ export enum MainProcessIpc {
 export enum WindowsManagerIpc {
  SignalUserInterfaceReadiness = 'windows-manager-signal-user-interface-readiness',
 }
+
+export const GRACEFUL_KILL_MESSAGE = 'GRACEFUL_KILL';


How about TERMINATE_MESSAGE instead? SIGKILL just ends a process with no hesitation, the graceful version of it has a name and it's SIGTERM. ;)

Also, please add a JSDoc explaining what the message signals.

gzdunek · 2024-07-26T15:06:04Z

I'm not sure how I feel about the graceful kill message through IPC, especially since we already have "infrastructure" in place to gracefully terminate tsh on Windows. OTOH, the way we kill tsh on Windows is a huge workaround, so perhaps it's best to stick to IPC which is guaranteed to work.

Yeah, I thought about it, but simplicity of process.send won :) (+ it's cross-platform)

github-actions · 2024-07-26T15:15:40Z

🤖 Vercel preview here: https://docs-7kiakc9sx-goteleport.vercel.app/docs/ver/preview

github-actions · 2024-07-29T09:22:51Z

🤖 Vercel preview here: https://docs-lf1zcxzj1-goteleport.vercel.app/docs/ver/preview

public-teleport-github-review-bot · 2024-07-29T09:36:24Z

@gzdunek See the table below for backport results.

Branch	Result
branch/v14	Failed
branch/v15	Failed
branch/v16	Create PR

* Add `terminal.windowsUseConpty` config option * Calculate `windowsPty` options * Pass `useConpty` to node-pty * Pass `windowsPty` to xterm * Fix test * Replace `terminal.windowsUseConpty` with `terminal.windowsBackend`, pass boolean all the way through * Wait for pty processes to exit before closing the app * Simplify `Array.from` * `GRACEFUL_KILL_MESSAGE` -> `TERMINATE_MESSAGE` * Adjust callsites to async `dispose` * Add `winpty` to ignored words in `cspell.json` (cherry picked from commit 981aed6)

gzdunek · 2024-07-29T14:38:21Z

I wanted to do a backport to v14, but there were too many conflicts.

* Add `terminal.windowsUseConpty` config option * Calculate `windowsPty` options * Pass `useConpty` to node-pty * Pass `windowsPty` to xterm * Fix test * Replace `terminal.windowsUseConpty` with `terminal.windowsBackend`, pass boolean all the way through * Wait for pty processes to exit before closing the app * Simplify `Array.from` * `GRACEFUL_KILL_MESSAGE` -> `TERMINATE_MESSAGE` * Adjust callsites to async `dispose` * Add `winpty` to ignored words in `cspell.json` (cherry picked from commit 981aed6)

gzdunek added 4 commits July 19, 2024 16:28

Add terminal.windowsUseConpty config option

814567d

Calculate windowsPty options

c63b463

Pass useConpty to node-pty

ae1c6cb

Pass windowsPty to xterm

3ff3ade

gzdunek added backport/branch/v14 backport/branch/v15 backport/branch/v16 labels Jul 19, 2024

gzdunek requested review from ravicious and ryanclark July 19, 2024 15:24

gzdunek temporarily deployed to vercel July 19, 2024 15:24 — with GitHub Actions Inactive

github-actions bot requested review from avatus, kimlisa, mmcallister, ptgott, r0mant, rudream, xinding33 and zmb3 July 19, 2024 15:25

github-actions bot added documentation size/md ui labels Jul 19, 2024

Fix test

6294749

gzdunek temporarily deployed to vercel July 19, 2024 15:32 — with GitHub Actions Inactive

gzdunek removed request for r0mant, avatus, mmcallister, xinding33, ptgott and kimlisa July 19, 2024 15:33

Replace terminal.windowsUseConpty with terminal.windowsBackend, p…

99e3c17

…ass boolean all the way through

gzdunek temporarily deployed to vercel July 23, 2024 09:57 — with GitHub Actions Inactive

ravicious approved these changes Jul 24, 2024

View reviewed changes

web/packages/teleterm/src/services/pty/ptyService.ts Outdated Show resolved Hide resolved

ravicious reviewed Jul 24, 2024

View reviewed changes

ravicious requested changes Jul 24, 2024

View reviewed changes

gzdunek requested a review from ravicious July 25, 2024 15:19

Wait for pty processes to exit before closing the app

2ae7285

gzdunek temporarily deployed to vercel July 25, 2024 15:24 — with GitHub Actions Inactive

ravicious approved these changes Jul 26, 2024

View reviewed changes

gzdunek added 3 commits July 26, 2024 16:34

Simplify Array.from

b5996ad

GRACEFUL_KILL_MESSAGE -> TERMINATE_MESSAGE

4abd7cc

Adjust callsites to async dispose

9189a20

gzdunek temporarily deployed to vercel July 26, 2024 15:05 — with GitHub Actions Inactive

Add winpty to ignored words in cspell.json

d26bf33

gzdunek temporarily deployed to vercel July 29, 2024 09:12 — with GitHub Actions Inactive

gzdunek added this pull request to the merge queue Jul 29, 2024

Merged via the queue into master with commit 981aed6 Jul 29, 2024
42 checks passed

gzdunek deleted the gzdunek/use-conpty-by-default branch July 29, 2024 09:34

This was referenced Jul 29, 2024

[v16] Enable ConPTY by default in terminals on Windows #44742

Merged

[v15] Enable ConPTY by default in terminals on Windows #44743

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable ConPTY by default in terminals on Windows #44468

Enable ConPTY by default in terminals on Windows #44468

gzdunek commented Jul 19, 2024 •

edited

Loading

github-actions bot commented Jul 19, 2024

github-actions bot commented Jul 23, 2024

ravicious Jul 24, 2024 •

edited

Loading

ravicious Jul 24, 2024 •

edited

Loading

gzdunek Jul 24, 2024 •

edited

Loading

ravicious Jul 24, 2024

gzdunek Jul 24, 2024

ravicious Jul 24, 2024

gzdunek Jul 25, 2024

gzdunek Jul 25, 2024

gzdunek Jul 25, 2024

ravicious left a comment

github-actions bot commented Jul 25, 2024

ravicious left a comment

ravicious Jul 26, 2024

gzdunek commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 29, 2024

public-teleport-github-review-bot bot commented Jul 29, 2024

gzdunek commented Jul 29, 2024

Enable ConPTY by default in terminals on Windows #44468

Enable ConPTY by default in terminals on Windows #44468

Conversation

gzdunek commented Jul 19, 2024 • edited Loading

Implementation

github-actions bot commented Jul 19, 2024

github-actions bot commented Jul 23, 2024

ravicious Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

ravicious Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

gzdunek Jul 24, 2024 • edited Loading

Choose a reason for hiding this comment

ravicious Jul 24, 2024

Choose a reason for hiding this comment

gzdunek Jul 24, 2024

Choose a reason for hiding this comment

ravicious Jul 24, 2024

Choose a reason for hiding this comment

gzdunek Jul 25, 2024

Choose a reason for hiding this comment

gzdunek Jul 25, 2024

Choose a reason for hiding this comment

gzdunek Jul 25, 2024

Choose a reason for hiding this comment

ravicious left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 25, 2024

ravicious left a comment

Choose a reason for hiding this comment

ravicious Jul 26, 2024

Choose a reason for hiding this comment

gzdunek commented Jul 26, 2024

github-actions bot commented Jul 26, 2024

github-actions bot commented Jul 29, 2024

public-teleport-github-review-bot bot commented Jul 29, 2024

gzdunek commented Jul 29, 2024

gzdunek commented Jul 19, 2024 •

edited

Loading

ravicious Jul 24, 2024 •

edited

Loading

ravicious Jul 24, 2024 •

edited

Loading

gzdunek Jul 24, 2024 •

edited

Loading