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

Fix unresponsive terminal in Connect on Windows Server 2019 #22971

Merged
merged 2 commits into from Mar 13, 2023

Conversation

ravicious
Copy link
Member

@ravicious ravicious commented Mar 13, 2023

Closes #22845.

Updating node-pty alone fixes the aforementioned issue. However, when running on Windows 11 with ConPTY enabled, node-pty would throw an uncaught exception when closing the shell by closing the tab (EPIPE, exactly the same as microsoft/node-pty#512). On top of that, closing the shell by executing exit would cause another uncaught exception, AttachConsole failed.

AttachConsole failed stack trace
C:\Users\rav\AppData\Local\Programs\teleport-connect\resources\app.asar\node_modules\node-pty\lib\conpty_console_list_agent.js:17
var consoleProcessList = getConsoleProcessList(shellPid);
                         ^

Error: AttachConsole failed
    at Object.<anonymous> (C:\Users\rav\AppData\Local\Programs\teleport-connect\resources\app.asar\node_modules\node-pty\lib\conpty_console_list_agent.js:17:26)
    at Module._compile (node:internal/modules/cjs/loader:1141:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1196:10)
    at Module.load (node:internal/modules/cjs/loader:1011:32)
    at Module._load (node:internal/modules/cjs/loader:846:12)
    at f._load (node:electron/js2c/asar_bundle:2:13328)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
    at node:internal/main/run_main_module:17:47

Those issues are not present on Windows Server 2019 running the same version of Connect with just node-pty updated (v11.3.8-dev.ravicious.2). Both problems are also not present on Windows 11 if ConPTY is not used.

node-pty uses ConPTY by default where available (as far as I know, this means both Windows 11 and Windows Server 2019).

Windows support is possible by utilizing the Windows conpty API on Windows 1809+ and the winpty library in older version.

The unfortunate downside of turning off ConPTY is that the winpty solution is worse at handling the terminal being resized. I think our best bet here is waiting for updates to ConPTY and node-pty and re-enable ConPTY in the future.


Backporting to v11 and v12 only because v10 uses an older Electron version and I didn't check if that version node-pty is compatible with it. We should aim to update the Electron version across all supported versions in the future.

@ravicious
Copy link
Member Author

VSCode uses another check to determine whether to enable ConPTY and they match on build 18309. My Windows Server 2019 instance has build number 17763.

I haven't tested that in Connect but I doubt this check would help much because the problem we have is with the new node-pty beta with ConPTY enabled on a recent Windows 11 version which build number is well after 18309.

@ravicious
Copy link
Member Author

Oh, and also I tested that update on Ubuntu and the terminal appears to be working just fine.

@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from gzdunek March 13, 2023 15:20
@ravicious ravicious requested a review from gzdunek March 13, 2023 16:46
@ravicious ravicious changed the base branch from ravicious/uncaught-exception to master March 13, 2023 16:55
@ravicious
Copy link
Member Author

Just rebased it on top of master instead of ravicious/uncaught-exception (#22962). This way I don't have to wait for it to get merged and in the end this PR doesn't depend on that one anyway.

@ravicious ravicious added this pull request to the merge queue Mar 13, 2023
Merged via the queue into master with commit a5fa148 Mar 13, 2023
@public-teleport-github-review-bot

@ravicious See the table below for backport results.

Branch Result
branch/v11 Create PR
branch/v12 Create PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Connect sessions are not accepting any user input
4 participants