Skip to content

std: Windows Console IO Fix #12400

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

Closed
wants to merge 6 commits into from

Conversation

Vulfox
Copy link
Contributor

@Vulfox Vulfox commented Aug 11, 2022

I tried tackling what I thought would be a somewhat small fix for these issues:

Resolves #5148
Resolves #7600

There were a couple of places I could make a solution work, but figured it would be best to introduce a new struct representing the std-out/err/in handles for Windows.

At first I was looking to change up write and read for the File struct with an if conditional on an isTty() check. It didn't seem right to add extra checks in these functions unnecessarily when I know that io.getStdXYZ() provides everything needed to know that the File or whatever returned type is meant for a Console on Windows. Doing so required some minimal changes to functions relying on getStdXYZ() to getStdXYZ().writer() or the equivalent for reader. On top of that, adding Windows specific fields to File also seemed like a bad idea for reading needs.

As far as not using GetConsoleMode or SetConsoleOutputCP for this, they seemed to have a risk of changing an active session's code page without any guarantees to the user that the program will set it back after it is finished or panics. I took a look at how Rust (may have borrowed some ideas from there) and Go implemented Windows console writing and they also didn't use these functions.

This is still a somewhat rough cut for a PR. I didn't want to make too many changes/additions without some input from maintainers. I can split this up into 2 different PRs for the Reader and Writer if need be, but I wanted to provide a fuller picture of what the changes look like for input and output.

Here's where I need some assistance/direction:

  • I don't quite know how I can test the new ConsoleReader+ConsoleWriter in the std lib. If File.(Read|Write)File has some Windows specific tests I can mimic and try to implement for reading from a console, I will happily add some there, but it doesn't seem like there are any specifically for those and Windows. I could be blind 🤷‍♂️
  • Is changing the getStdXYZ functions the best place for introducing this logic of translating utf8<->utf16 for Windows Console?
  • Is std/io the preferred directory for the Windows specific reader and writer to live?

I was able to eye ball test these changes locally on Windows 10 and can confirm what used to return junk characters, now properly renders the correct unicode characters on the terminal.

Copy link
Contributor

@wooster0 wooster0 left a comment

Choose a reason for hiding this comment

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

Just my 2 cents.

const is_windows = builtin.os.tag == .windows;

threadlocal var win_surrogate: ?u16 = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm after thinking a bit more about this, I'm not sure this should be threadlocal after all. What I'd propose would be:

  • Leave the buffer as a static global (non-threadlocal)
  • Any use of stdin across threads on Windows requires you to synchronize I/O access using your own mutex or similar

Do you see any issues with that approach? cc @kprotty

Copy link
Member

Choose a reason for hiding this comment

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

Its an odd restriction for only a single platform. If it does exist, I think it should extend to all platforms and even all tty Files

Copy link
Contributor Author

@Vulfox Vulfox Aug 24, 2022

Choose a reason for hiding this comment

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

@topolarity I've been mulling over the implications of threadlocal as well. Your suggestion sounds like exactly what the proposal I mentioned before definitely wanted to maybe tackle.

@kprotty I can try to see what I can cook up to settle this in a platform agnostic way, while trying to dodge the bullet of changing any current public APIs/types.

edit: Y'all got any recommended material that might help me get started down the mutex buffer path for best practices?

Copy link
Contributor

Choose a reason for hiding this comment

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

My thought was to require the user to synchronize their use of stdout/stdin with their own mutex, not to wrap this in a mutex in the standard library

That would leave the default behavior fast for single-threaded usage, while still making it possible to perform multi-threaded access correctly (multi-threaded access is broken with threadlocal, since each thread will see a different buffer)

@andrewrk
Copy link
Member

@Vulfox could you please rebase this on latest master? My auto-rebase script failed due to conflicts.

@andrewrk
Copy link
Member

Unfortunately, this needs another rebase - CI tests didn't pass last time, and now there are conflicts with master branch.

@andrewrk
Copy link
Member

Closing - no updates for 30 days and CI checks are failing. Please open a new PR if you want to continue these efforts.

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