Refactor Windows stdio and remove stdin double buffering #58454

pitdicker · 2019-02-14T07:18:28Z

I was looking for something nice and small to work on, tried to tackle a few FIXME's in Windows stdio, and things grew from there.

This part of the standard library contains some tricky code, and has changed over the years to handle more corner cases. It could use some refactoring and extra comments.

Changes/fixes:

Made StderrRaw pub(crate), to remove the Write implementations on sys::Stderr (used unsynchronised for panic output).
Remove the unused Read implementation on sys::windows::stdin
The windows::stdio::Output enum made sense when we cached the handles, but we can use simple functions like is_console now that we get the handle on every read/write
write can now calculate the number of written bytes as UTF-8 when we can't write all u16s.
If write could only write one half of a surrogate pair, attempt another write for the other because user code can't reslice in any way that would allow us to write it otherwise.
Removed the double buffering on stdin. Documentation on the unexposed StdinRaw says: 'This handle is not synchronized or buffered in any fashion'; which is now true.
sys::windows::Stdin now always only partially fills its buffer, so we can guarantee any arbitrary UTF-16 can be re-encoded without losing any data.
sys::windows::STDIN_BUF_SIZE is slightly larger to compensate. There should be no real change in the number of syscalls the buffered Stdin does. This buffer is a little larger, while the extra buffer on Stdin is gone.
sys::windows::Stdin now attempts to handle unpaired surrogates at its buffer boundary.
sys::windows::Stdin no langer allocates for its buffer, but the UTF-16 decoding still does.

Testing

I did some manual testing of reading and writing to console. The console does support UTF-16 in some sense, but doesn't supporting displaying characters outside the BMP.

compile stage 1 stdlib with a tiny value for MAX_BUFFER_SIZE to make it easier to catch corner cases
run a simple test program that reads on stdin, and echo's to stdout
write some lines with plenty of ASCII and emoji in a text editor
copy and paste in console to stdin
return with \r\n\ or CTRL-Z
copy and paste in text editor
check it round-trips

Fixes #23344. All but one of the suggestions in that issue are now implemented. the missing one is:

When reading data, we require the entire set of input to be valid UTF-16. We should instead attempt to read as much of the input as possible as valid UTF-16, only returning an error for the actual invalid elements. For example if we read 10 elements, 5 of which are valid UTF-16, the 6th is bad, and then the remaining are all valid UTF-16, we should probably return the first 5 on a call to read, then return an error, then return the remaining on the next call to read.

Stdin in Console mode is dealing with text directly input by a user. In my opinion getting an unpaired surrogate is quite unlikely in that case, and a valid reason to error on the entire line of input (which is probably short). Dealing with it is incompatible with an unbuffered stdin, which seems the more interesting guarantee to me.

rust-highfive · 2019-02-14T07:18:31Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

Centril · 2019-02-14T07:23:48Z

cc @retep998

src/libstd/sys/windows/stdio.rs

cramertj · 2019-02-14T18:00:36Z

r? @alexcrichton

pitdicker · 2019-02-15T11:14:46Z

Did a forced push: my changes didn't handle well when ReadConsoleW reports an amount of 0. It tried to reslice with [..amount] and look at the last character, now it checks for the amount first.

alexcrichton

Thanks so much for the PR @pitdicker! It's always quite satisfying to have such a blast from the past and see some old code getting some well-deserved love.

I'm mostly curious about in your testing how many of these edge cases you were able to trigger? Some of the handling here and there seems like it would do well with some tests (not that I have any idea how we could test any of this in an automated fashion), but it all looks overally quite solid to me and a great improvement from before.

src/libstd/sys/windows/stdio.rs

alexcrichton · 2019-02-15T16:08:25Z

src/libstd/sys/windows/stdio.rs

+    let mut written = write_u16s(handle, &utf16)?;
+
+    // Figure out how many bytes of as UTF-8 were written away as UTF-16.
+    if written >= utf16.len() {


Is the > here of >= for safety? It seems like it'd be surprising if > happened!

It is not really useful no. It helps to prove with the indexing bounds checking in the else arm. Not sure that is worth the question it can cause when reading the code.

Heh ok! This is pretty far off the performance critical path so I'm not too worried about that, so I'd be fine with an assert here or something like that

src/libstd/sys/windows/stdio.rs

pitdicker · 2019-02-15T16:56:15Z

Thank you for the extensive review. It may take me a few days to reply to everything though...

pitdicker · 2019-02-15T18:48:29Z

I wonder if it is still useful to wrap the Std*Raw types in Maybe<..>. It was introduced in #26168 and RFC 1014. Now that the handle is acquired on every read and write, it seems easier to handle it all in sys::windows::stdio. That would also remove another corner case for if we ever want to expose the raw types.
edit: Ignore me, there seem to be more cases where stdio can be missing and Maybe is desired.

pitdicker · 2019-02-20T19:55:44Z

I have some trouble roundtripping a long line of emoji, will report back when ready.

pitdicker · 2019-02-21T07:20:00Z

The problem seems to be with Console and Powershell. In a long string of emoji it sometimes inserts an extra space. This happens with the changes here, the previous version of the code, when manually writing out u16s (verified to not contain spaces), and even when pasting it at the command prompt and getting the same string back as a command not found error.

@alexcrichton Now that you're here, do you know it this comment is still relevant? Or would changing the buffer strategy of Stdout be too big of a change as it is a few years later?

alexcrichton · 2019-02-21T16:46:29Z

Interesting! And also odd...

In any case that comment should be fine to fix (although it likely needs to be done carefully). I think that we'll want to have that as a separate PR from this one.

Is this one ready for another look-over?

pitdicker · 2019-02-21T17:17:13Z

In any case that comment should be fine to fix (although it likely needs to be done carefully). I think that we'll want to have that as a separate PR from this one.

Maybe I'll give it a try.

Is this one ready for another look-over?

Yes.

alexcrichton

This all looks fantastic to me, thanks so much again for this @pitdicker!

I'm curious on your thoughts on the minimum 4-byte buffer, but otherwise r=me

alexcrichton · 2019-02-21T19:06:42Z

src/libstd/sys/windows/stdio.rs

+        if buf.len() == 0 {
+            return Ok(0);
+        } else if buf.len() < 4 {
+            return Err(io::Error::new(io::ErrorKind::InvalidInput,


FWIW this seems somewhat bad in the sense that this should handle arbitrary sized buffers, but I'm not too worried about this as it seems like this is vanishingly rare to come up in practice (e.g. libstd doesn't actually expose a way to do it due to its buffered reads I think), and we can always solve it with a 4-byte buffer in Stdin and some extra logic down the road.

I thought about this case if StdinRaw is ever exposed. Read::read_exact continues to give a smaller slice of the buffer until it is completely filled, and is just about guaranteed to hit this error. But I can't imagine any reasonable use for read_exact on an unbuffered stdin.

and we can always solve it with a 4-byte buffer in Stdin and some extra logic down the road.

Seems like a solution when unbuffered reads get exposed. But feels a bit to me like saying: you want to have unbuffered access to stdin? Here it is, and with a tiny buffer to make sure you can do arbitrary small reads. Not sure how to say it, it feels more like the concern of a wrapper.

Yeah sounds good to me. If we leave this we'll just have to be sure to document it, but I think we can avoid that for now.

alexcrichton · 2019-02-21T19:52:42Z

@bors: r+

bors · 2019-02-21T19:52:43Z

📌 Commit 6464e32 has been approved by alexcrichton

pitdicker · 2019-02-21T19:54:11Z

Thank you!

…hton Refactor Windows stdio and remove stdin double buffering I was looking for something nice and small to work on, tried to tackle a few FIXME's in Windows stdio, and things grew from there. This part of the standard library contains some tricky code, and has changed over the years to handle more corner cases. It could use some refactoring and extra comments. Changes/fixes: - Made `StderrRaw` `pub(crate)`, to remove the `Write` implementations on `sys::Stderr` (used unsynchronised for panic output). - Remove the unused `Read` implementation on `sys::windows::stdin` - The `windows::stdio::Output` enum made sense when we cached the handles, but we can use simple functions like `is_console` now that we get the handle on every read/write - `write` can now calculate the number of written bytes as UTF-8 when we can't write all `u16`s. - If `write` could only write one half of a surrogate pair, attempt another write for the other because user code can't reslice in any way that would allow us to write it otherwise. - Removed the double buffering on stdin. Documentation on the unexposed `StdinRaw` says: 'This handle is not synchronized or buffered in any fashion'; which is now true. - `sys::windows::Stdin` now always only partially fills its buffer, so we can guarantee any arbitrary UTF-16 can be re-encoded without losing any data. - `sys::windows::STDIN_BUF_SIZE` is slightly larger to compensate. There should be no real change in the number of syscalls the buffered `Stdin` does. This buffer is a little larger, while the extra buffer on Stdin is gone. - `sys::windows::Stdin` now attempts to handle unpaired surrogates at its buffer boundary. - `sys::windows::Stdin` no langer allocates for its buffer, but the UTF-16 decoding still does. ### Testing I did some manual testing of reading and writing to console. The console does support UTF-16 in some sense, but doesn't supporting displaying characters outside the BMP. - compile stage 1 stdlib with a tiny value for `MAX_BUFFER_SIZE` to make it easier to catch corner cases - run a simple test program that reads on stdin, and echo's to stdout - write some lines with plenty of ASCII and emoji in a text editor - copy and paste in console to stdin - return with `\r\n\` or CTRL-Z - copy and paste in text editor - check it round-trips ----- Fixes rust-lang#23344. All but one of the suggestions in that issue are now implemented. the missing one is: > * When reading data, we require the entire set of input to be valid UTF-16. We should instead attempt to read as much of the input as possible as valid UTF-16, only returning an error for the actual invalid elements. For example if we read 10 elements, 5 of which are valid UTF-16, the 6th is bad, and then the remaining are all valid UTF-16, we should probably return the first 5 on a call to `read`, then return an error, then return the remaining on the next call to `read`. Stdin in Console mode is dealing with text directly input by a user. In my opinion getting an unpaired surrogate is quite unlikely in that case, and a valid reason to error on the entire line of input (which is probably short). Dealing with it is incompatible with an unbuffered stdin, which seems the more interesting guarantee to me.

Centril · 2019-02-23T04:06:36Z

Failed in #58665 (comment), @bors r-

Centril · 2019-02-23T04:07:05Z

src/libstd/io/mod.rs

@@ -286,6 +286,9 @@ pub use self::stdio::{_print, _eprint};
 #[doc(no_inline, hidden)]
 pub use self::stdio::{set_panic, set_print};

+// Used inside the standard library for panic output.
+pub(crate) use self::stdio::stderr_raw;


Offending line (unused import)

pitdicker · 2019-02-23T11:34:02Z

@Centril Apologies for breaking two rollups!

Worst thing is that the pub (crate) part was not even necessary anymore now that all the sys types implement Read/Write, and I was preparing a commit to revert it in a follow-up PR.

alexcrichton · 2019-02-23T16:07:56Z

@bors: r+

bors · 2019-02-23T16:07:56Z

📌 Commit 1a944b0 has been approved by alexcrichton

…hton Refactor Windows stdio and remove stdin double buffering I was looking for something nice and small to work on, tried to tackle a few FIXME's in Windows stdio, and things grew from there. This part of the standard library contains some tricky code, and has changed over the years to handle more corner cases. It could use some refactoring and extra comments. Changes/fixes: - Made `StderrRaw` `pub(crate)`, to remove the `Write` implementations on `sys::Stderr` (used unsynchronised for panic output). - Remove the unused `Read` implementation on `sys::windows::stdin` - The `windows::stdio::Output` enum made sense when we cached the handles, but we can use simple functions like `is_console` now that we get the handle on every read/write - `write` can now calculate the number of written bytes as UTF-8 when we can't write all `u16`s. - If `write` could only write one half of a surrogate pair, attempt another write for the other because user code can't reslice in any way that would allow us to write it otherwise. - Removed the double buffering on stdin. Documentation on the unexposed `StdinRaw` says: 'This handle is not synchronized or buffered in any fashion'; which is now true. - `sys::windows::Stdin` now always only partially fills its buffer, so we can guarantee any arbitrary UTF-16 can be re-encoded without losing any data. - `sys::windows::STDIN_BUF_SIZE` is slightly larger to compensate. There should be no real change in the number of syscalls the buffered `Stdin` does. This buffer is a little larger, while the extra buffer on Stdin is gone. - `sys::windows::Stdin` now attempts to handle unpaired surrogates at its buffer boundary. - `sys::windows::Stdin` no langer allocates for its buffer, but the UTF-16 decoding still does. ### Testing I did some manual testing of reading and writing to console. The console does support UTF-16 in some sense, but doesn't supporting displaying characters outside the BMP. - compile stage 1 stdlib with a tiny value for `MAX_BUFFER_SIZE` to make it easier to catch corner cases - run a simple test program that reads on stdin, and echo's to stdout - write some lines with plenty of ASCII and emoji in a text editor - copy and paste in console to stdin - return with `\r\n\` or CTRL-Z - copy and paste in text editor - check it round-trips ----- Fixes rust-lang#23344. All but one of the suggestions in that issue are now implemented. the missing one is: > * When reading data, we require the entire set of input to be valid UTF-16. We should instead attempt to read as much of the input as possible as valid UTF-16, only returning an error for the actual invalid elements. For example if we read 10 elements, 5 of which are valid UTF-16, the 6th is bad, and then the remaining are all valid UTF-16, we should probably return the first 5 on a call to `read`, then return an error, then return the remaining on the next call to `read`. Stdin in Console mode is dealing with text directly input by a user. In my opinion getting an unpaired surrogate is quite unlikely in that case, and a valid reason to error on the entire line of input (which is probably short). Dealing with it is incompatible with an unbuffered stdin, which seems the more interesting guarantee to me.

@ghost

Rollup of 6 pull requests Successful merges: - #57364 (Improve parsing diagnostic for negative supertrait bounds) - #58183 (Clarify guarantees for `Box` allocation) - #58442 (Simplify the unix `Weak` functionality) - #58454 (Refactor Windows stdio and remove stdin double buffering ) - #58511 (Const to op simplification) - #58642 (rustdoc: support methods on primitives in intra-doc links) Failed merges: r? @ghost

rust-highfive assigned cramertj Feb 14, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 14, 2019

iancormac84 reviewed Feb 14, 2019

View reviewed changes

src/libstd/sys/windows/stdio.rs Outdated Show resolved Hide resolved

rust-highfive assigned alexcrichton and unassigned cramertj Feb 14, 2019

alexcrichton reviewed Feb 15, 2019

View reviewed changes

pitdicker mentioned this pull request Feb 15, 2019

Expose raw Stdout/err/in #58326

Open

pitdicker added 5 commits February 20, 2019 06:37

Remove sys::*::Stderr Write implementation

0651157

Remove unused Read implementation on sys::Windows::Stdin

cc20ed6

Refactor Windows stdio and remove stdin double buffering

f411852

Address review comments

b09803e

Use standard Read/Write traits in sys::stdio

6464e32

alexcrichton approved these changes Feb 21, 2019

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 21, 2019

Centril mentioned this pull request Feb 23, 2019

Rollup of 16 pull requests #58664

Closed

Centril mentioned this pull request Feb 23, 2019

Rollup of 16 pull requests #58665

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 23, 2019

Centril reviewed Feb 23, 2019

View reviewed changes

Remove pub(crate) from stderr_raw

1a944b0

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 23, 2019

Centril mentioned this pull request Feb 24, 2019

Rollup of 6 pull requests #58688

Closed

Centril mentioned this pull request Feb 24, 2019

Rollup of 6 pull requests #58691

Merged

bors merged commit 1a944b0 into rust-lang:master Feb 24, 2019

pitdicker deleted the windows_stdio branch March 1, 2019 19:27

pitdicker mentioned this pull request Dec 10, 2021

Modifications to std::io::Stdin on Windows so that there is no longer a 4-byte buffer minimum in read(). #91754

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Windows stdio and remove stdin double buffering #58454

Refactor Windows stdio and remove stdin double buffering #58454

pitdicker commented Feb 14, 2019 •

edited

Loading

rust-highfive commented Feb 14, 2019

Centril commented Feb 14, 2019

cramertj commented Feb 14, 2019

pitdicker commented Feb 15, 2019

alexcrichton left a comment

alexcrichton Feb 15, 2019

pitdicker Feb 15, 2019

alexcrichton Feb 15, 2019

pitdicker commented Feb 15, 2019

pitdicker commented Feb 15, 2019 •

edited

Loading

pitdicker commented Feb 20, 2019

pitdicker commented Feb 21, 2019

alexcrichton commented Feb 21, 2019

pitdicker commented Feb 21, 2019

alexcrichton left a comment

alexcrichton Feb 21, 2019

pitdicker Feb 21, 2019

alexcrichton Feb 21, 2019

alexcrichton commented Feb 21, 2019

bors commented Feb 21, 2019

pitdicker commented Feb 21, 2019

Centril commented Feb 23, 2019

Centril Feb 23, 2019

pitdicker commented Feb 23, 2019

alexcrichton commented Feb 23, 2019

bors commented Feb 23, 2019

Refactor Windows stdio and remove stdin double buffering #58454

Refactor Windows stdio and remove stdin double buffering #58454

Conversation

pitdicker commented Feb 14, 2019 • edited Loading

Testing

rust-highfive commented Feb 14, 2019

Centril commented Feb 14, 2019

cramertj commented Feb 14, 2019

pitdicker commented Feb 15, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Feb 15, 2019

Choose a reason for hiding this comment

pitdicker Feb 15, 2019

Choose a reason for hiding this comment

alexcrichton Feb 15, 2019

Choose a reason for hiding this comment

pitdicker commented Feb 15, 2019

pitdicker commented Feb 15, 2019 • edited Loading

pitdicker commented Feb 20, 2019

pitdicker commented Feb 21, 2019

alexcrichton commented Feb 21, 2019

pitdicker commented Feb 21, 2019

alexcrichton left a comment

Choose a reason for hiding this comment

alexcrichton Feb 21, 2019

Choose a reason for hiding this comment

pitdicker Feb 21, 2019

Choose a reason for hiding this comment

alexcrichton Feb 21, 2019

Choose a reason for hiding this comment

alexcrichton commented Feb 21, 2019

bors commented Feb 21, 2019

pitdicker commented Feb 21, 2019

Centril commented Feb 23, 2019

Centril Feb 23, 2019

Choose a reason for hiding this comment

pitdicker commented Feb 23, 2019

alexcrichton commented Feb 23, 2019

bors commented Feb 23, 2019

pitdicker commented Feb 14, 2019 •

edited

Loading

pitdicker commented Feb 15, 2019 •

edited

Loading