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(ext/node): windows cancel stdin read in line mode #23969

Merged
merged 5 commits into from
May 29, 2024

Conversation

littledivy
Copy link
Member

This patch fixes stdin read hanging on user input when switching tty mode on Windows

Fixes #21111

On Windows, when switching from line to raw mode:

  • Cancel ongoing console read by writing a return keypress to its input buffer. This blocks the main thread until any ongoing read has been cancelled to prevent interference with the screen state.
  • On the read thread, restore the cursor position to where it was before writing the enter, undoing its effect on the screen state.
  • Restart reading and notify the main thread.

@littledivy littledivy marked this pull request as ready for review May 24, 2024 09:36
@littledivy littledivy force-pushed the win_tty_read_cancel branch 3 times, most recently from 6dc4ead to 5c15e0c Compare May 24, 2024 10:52
Copy link
Member

@satyarohith satyarohith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have a Windows machine to test the change. I'll let David review the PR.

Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, looks good to me. Nice that there's a test. @dsherret can you try it out on your machine and see if it fixes the problem at hand?

ext/io/lib.rs Outdated Show resolved Hide resolved
Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ext/io/lib.rs Outdated Show resolved Hide resolved
ext/io/lib.rs Outdated Show resolved Hide resolved
ext/io/lib.rs Outdated Show resolved Hide resolved
ext/io/lib.rs Outdated Show resolved Hide resolved
@piscisaureus
Copy link
Member

Probably the most terrible windows hack of all time.

I remember that in libuv we had to replace an already-terrible hack (calling ReadConsoleW against a duplicate of the CONIN$ handle; closing that handle would cancel the cooked read) that did the trick for Windows pre-Vista, but that stopped working when Windows 7 rolled around, leaving us with not other options.

Pretty appalling that more than a decade later there is still no solution on the horizon: microsoft/terminal#12143.

  • On the read thread, restore the cursor position to where it was before writing the enter, undoing its effect on the screen state.
  • Since in cooked mode the terminal will echo back any characters you write to it, you have to coordinate this action not just with reads but also with console writes.
  • You have to consider what happens when the cursor position is at the very bottom of the buffer and the "enter", which will cause entire screen buffer to scroll while the cursor remains on the last line.

Final note: Best you can do is try it in practice and see that it works OK most of the time. It probably always will have edge cases where text ends up garbled. Only Microsoft could fix this (they won't though).

This patch fixes stdin read hanging on user input when switching tty mode on Windows

Fixes #21111

On Windows, when switching from line to raw mode:

  • Cancel ongoing console read by writing a return keypress to its input buffer. This blocks the main thread until any ongoing read has been cancelled to prevent interference with the screen state.
  • On the read thread, restore the cursor position to where it was before writing the enter, undoing its effect on the screen state.
  • Restart reading and notify the main thread.

@littledivy
Copy link
Member Author

Best you can do is try it in practice and see that it works OK most of the time.

Node v Deno comparision for some packages:

create-next-app

recording.mp4

crate-solid

solid.mp4

create-vite:

vite.mp4

Seems to work most of the time :)

Copy link
Member

@dsherret dsherret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@littledivy littledivy merged commit a947c6f into denoland:main May 29, 2024
17 checks passed
@littledivy littledivy deleted the win_tty_read_cancel branch May 29, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

create-next-app prompts don't work on Windows
5 participants