Optimize File::read_to_end and read_to_string
Reading a file into an empty vector or string buffer can incur unnecessary `read` syscalls and memory re-allocations as the buffer "warms up" and grows to its final size. This is perhaps a necessary evil with generic readers, but files can be read in smarter by checking the file size and reserving that much capacity.
`std::fs::read` and `std::fs::read_to_string` already perform this optimization: they open the file, reads its metadata, and call `with_capacity` with the file size. This ensures that the buffer does not need to be resized and an initial string of small `read` syscalls.
However, if a user opens the `File` themselves and calls `file.read_to_end` or `file.read_to_string` they do not get this optimization.
```rust
let mut buf = Vec::new();
file.read_to_end(&mut buf)?;
```
I searched through this project's codebase and even here are a *lot* of examples of this. They're found all over in unit tests, which isn't a big deal, but there are also several real instances in the compiler and in Cargo. I've documented the ones I found in a comment here:
https://github.com/rust-lang/rust/issues/89516#issuecomment-934423999
Most telling, the documentation for both the `Read` trait and the `Read::read_to_end` method both show this exact pattern as examples of how to use readers. What this says to me is that this shouldn't be solved by simply fixing the instances of it in this codebase. If it's here it's certain to be prevalent in the wider Rust ecosystem.
To that end, this commit adds specializations of `read_to_end` and `read_to_string` directly on `File`. This way it's no longer a minor footgun to start with an empty buffer when reading a file in.
A nice side effect of this change is that code that accesses a `File` as `impl Read` or `dyn Read` will benefit. For example, this code from `compiler/rustc_serialize/src/json.rs`:
```rust
pub fn from_reader(rdr: &mut dyn Read) -> Result<Json, BuilderError> {
let mut contents = Vec::new();
match rdr.read_to_end(&mut contents) {
```
Related changes:
- I also added specializations to `BufReader` to delegate to `self.inner`'s methods. That way it can call `File`'s optimized implementations if the inner reader is a file.
- The private `std::io::append_to_string` function is now marked `unsafe`.
- `File::read_to_string` being more efficient means that the performance note for `io::read_to_string` can be softened. I've added `@camelid's` suggested wording from https://github.com/rust-lang/rust/issues/80218#issuecomment-936806502.
r? `@joshtriplett`
Reading a file into an empty vector or string buffer can incur
unnecessary `read` syscalls and memory re-allocations as the buffer
"warms up" and grows to its final size. This is perhaps a necessary evil
with generic readers, but files can be read in smarter by checking the
file size and reserving that much capacity.
`std::fs::read` and `read_to_string` already perform this optimization:
they open the file, reads its metadata, and call `with_capacity` with
the file size. This ensures that the buffer does not need to be resized
and an initial string of small `read` syscalls.
However, if a user opens the `File` themselves and calls
`file.read_to_end` or `file.read_to_string` they do not get this
optimization.
```rust
let mut buf = Vec::new();
file.read_to_end(&mut buf)?;
```
I searched through this project's codebase and even here are a *lot* of
examples of this. They're found all over in unit tests, which isn't a
big deal, but there are also several real instances in the compiler and
in Cargo. I've documented the ones I found in a comment here:
https://github.com/rust-lang/rust/issues/89516#issuecomment-934423999
Most telling, the `Read` trait and the `read_to_end` method both show
this exact pattern as examples of how to use readers. What this says to
me is that this shouldn't be solved by simply fixing the instances of it
in this codebase. If it's here it's certain to be prevalent in the wider
Rust ecosystem.
To that end, this commit adds specializations of `read_to_end` and
`read_to_string` directly on `File`. This way it's no longer a minor
footgun to start with an empty buffer when reading a file in.
A nice side effect of this change is that code that accesses a `File` as
a bare `Read` constraint or via a `dyn Read` trait object will benefit.
For example, this code from `compiler/rustc_serialize/src/json.rs`:
```rust
pub fn from_reader(rdr: &mut dyn Read) -> Result<Json, BuilderError> {
let mut contents = Vec::new();
match rdr.read_to_end(&mut contents) {
```
Related changes:
- I also added specializations to `BufReader` to delegate to
`self.inner`'s methods. That way it can call `File`'s optimized
implementations if the inner reader is a file.
- The private `std::io::append_to_string` function is now marked
`unsafe`.
- `File::read_to_string` being more efficient means that the performance
note for `io::read_to_string` can be softened. I've added @camelid's
suggested wording from:
https://github.com/rust-lang/rust/issues/80218#issuecomment-936806502
Fix read_to_end to not grow an exact size buffer
If you know how much data to expect and use `Vec::with_capacity` to pre-allocate a buffer of that capacity, `Read::read_to_end` will still double its capacity. It needs some space to perform a read, even though that read ends up returning `0`.
It's a bummer to carefully pre-allocate 1GB to read a 1GB file into memory and end up using 2GB.
This fixes that behavior by special casing a full buffer and reading into a small "probe" buffer instead. If that read returns `0` then it's confirmed that the buffer was the perfect size. If it doesn't, the probe buffer is appended to the normal buffer and the read loop continues.
Fixing this allows several workarounds in the standard library to be removed:
- `Take` no longer needs to override `Read::read_to_end`.
- The `reservation_size` callback that allowed `Take` to inhibit the previous over-allocation behavior isn't needed.
- `fs::read` doesn't need to reserve an extra byte in `initial_buffer_size`.
Curiously, there was a unit test that specifically checked that `Read::read_to_end` *does* over-allocate. I removed that test, too.
Fix spacing of links in inline code.
Similar to #80733, but the focus is different. This PR eliminates all occurrences of pieced-together inline code blocks like [`Box`]`<`[`Option`]`<T>>` and replaces them with good-looking ones (using HTML-syntax), like <code>[Box]<[Option]\<T>></code>. As far as I can tell, I should’ve found all of these in the standard library (regex search with `` r"`\]`|`\[`" ``) \[except for in `core::convert` where I’ve noticed other things in the docs that I want to fix in a separate PR]. In particular, unlike #80733, I’ve added almost no new instance of inline code that’s broken up into multiple links (or some link and some link-free part). I also added tooltips (the stuff in quotes for the markdown link listings) in places that caught my eye, but that’s by no means systematic, just opportunistic.
[Box]: https://doc.rust-lang.org/std/boxed/struct.Box.html "Box"
[`Box`]: https://doc.rust-lang.org/std/boxed/struct.Box.html "Box"
[Option]: https://doc.rust-lang.org/std/option/enum.Option.html "Option"
[`Option`]: https://doc.rust-lang.org/std/option/enum.Option.html "Option"
Context: I got annoyed by repeatedly running into new misformatted inline code while reading the standard library docs. I know that once issue #83997 (and/or related ones) are resolved, these changes become somewhat obsolete, but I fail to notice much progress on that end right now.
r? `@jyn514`
----------
Fix spacing for links inside code blocks, and improve link tooltips in alloc::fmt
----------
Fix spacing for links inside code blocks, and improve link tooltips in alloc::{rc, sync}
----------
Fix spacing for links inside code blocks, and improve link tooltips in alloc::string
----------
Fix spacing for links inside code blocks in alloc::vec
----------
Fix spacing for links inside code blocks in core::option
----------
Fix spacing for links inside code blocks, and improve a few link tooltips in core::result
----------
Fix spacing for links inside code blocks in core::{iter::{self, iterator}, stream::stream, poll}
----------
Fix spacing for links inside code blocks, and improve a few link tooltips in std::{fs, path}
----------
Fix spacing for links inside code blocks in std::{collections, time}
----------
Fix spacing for links inside code blocks in and make formatting of `&str`-like types consistent in std::ffi::{c_str, os_str}
----------
Fix spacing for links inside code blocks, and improve link tooltips in std::ffi
----------
Fix spacing for links inside code blocks, and improve a few link tooltips
in std::{io::{self, buffered::{bufreader, bufwriter}, cursor, util}, net::{self, addr}}
----------
Fix typo in link to `into` for `OsString` docs
----------
Remove tooltips that will probably become redundant in the future
----------
Apply suggestions from code review
Replacing `…std/primitive.reference.html` paths with just `reference`
Co-authored-by: Joshua Nelson <github@jyn.dev>
----------
Also replace `…std/primitive.reference.html` paths with just `reference` in `core::pin`
If you know how much data to expect and use `Vec::with_capacity` to
pre-allocate a buffer of that capacity, `Read::read_to_end` will still
double its capacity. It needs some space to perform a read, even though
that read ends up returning `0`.
It's a bummer to carefully pre-allocate 1GB to read a 1GB file into
memory and end up using 2GB.
This fixes that behavior by special casing a full buffer and reading
into a small "probe" buffer instead. If that read returns `0` then it's
confirmed that the buffer was the perfect size. If it doesn't, the probe
buffer is appended to the normal buffer and the read loop continues.
Fixing this allows several workarounds in the standard library to be
removed:
- `Take` no longer needs to override `Read::read_to_end`.
- The `reservation_size` callback that allowed `Take` to inhibit the
previous over-allocation behavior isn't needed.
- `fs::read` doesn't need to reserve an extra byte in
`initial_buffer_size`.
Curiously, there was a unit test that specifically checked that
`Read::read_to_end` *does* over-allocate. I removed that test, too.
Stabilise BufWriter::into_parts
The FCP for this has already completed, in #80690.
This was just blocked on #85901 (which changed the name), which is now merged. The original stabilisation MR was #84770 but that has a lot of noise in it, and I also accidentally deleted the branch while trying to tidy up. So here is a new MR. Sorry for the noise.
Closes#80690
It is useful to keep some coherent structure to this ordering. In
particular, Other and Uncategorized should be next to each other, at
the end.
Also it seems to make sense to treat UnexpectedEof and OutOfMemory
specially, since they are not like the other errors (despite
OutOfMemory also being generatable by some OS errors).
So:
* Move Other to the end, just before Uncategorized
* Move Unsupported to between Interrupted and UnexpectedEof
* Add some comments documenting where to add things
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Replace read_to_string with read_line in Stdin example
The current example results in infinitely reading from stdin, which can confuse newcomers trying to read from stdin.
(`@razmag` encountered this while learning the language from the docs)
impl Default, Copy, Clone for std::io::Sink and Empty
The omission of `Sink: Default` is causing me a slight inconvenience in a test harness. There seems little reason for this and `Empty` not to be `Clone` and `Copy` too.
I have made all three of these insta-stable, because:
AIUI `Copy` can only be derived, and I was not able to find any examples of how to unstably derive it. I think it is probably not possible.
I hunted through the git history for precedent and found
> 79b8ad84c8
> Implement `Copy` for `IoSlice`
> https://github.com/rust-lang/rust/pull/69403
which was also insta-stable.
Fix may not to appropriate might not or must not
I went through and changed occurrences of `may not` to be more explicit with `might not` and `must not`.
I looked in stdlib and as @BurntSushi thought, `raw` is generally
used for raw pointers, or other hazardous kinds of thing. stdlib does
not have `into_parts` apart from the one I added to `IntoInnerError`.
I did an ad-hoc search of the rustdocs for my current game project
Otter, which includes quite a large number of dependencies.
`into_parts` seems heavily used for things quite like this.
So change this name.
Suggested-by: Andrew Gallant <jamslam@gmail.com>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
I didn't notice the submodule, which means I failed to re-export this
to make it actually-public.
Reported-by: Andrew Gallant <jamslam@gmail.com>
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Stabilize `into_parts()` and `into_error()`
This stabilizes `IntoInnerError`'s `into_parts()` and `into_error()` methods, currently gated behind the `io_into_inner_error_parts` feature. The FCP has [already completed.](https://github.com/rust-lang/rust/issues/79704#issuecomment-880652967)
Closes#79704.
add `Stdin::lines`, `Stdin::split` forwarder methods
Add forwarder methods `Stdin::lines` and `Stdin::split`, which consume
and lock a `Stdin` handle, and forward on to the corresponding `BufRead`
methods. This should make it easier for beginners to use those iterator
constructors without explicitly dealing with locks or lifetimes.
Replaces #86412.
~~Based on #86846 to get the tracking issue number for the `stdio_locked` feature.~~ Rebased after merge, so it's only one commit now.
r? `@joshtriplett`
`@rustbot` label +A-io +C-enhancement +D-newcomer-roadblock +T-libs-api
Add diagnostic items for Clippy
This adds a bunch of diagnostic items to `std`/`core`/`alloc` functions, structs and traits used in Clippy. The actual refactorings in Clippy to use these items will be done in a different PR in Clippy after the next sync.
This PR doesn't include all paths Clippy uses, I've only gone through the first 85 lines of Clippy's [`paths.rs`](ecf85f4bdc/clippy_utils/src/paths.rs) (after rust-lang/rust-clippy#7466) to get some feedback early on. I've also decided against adding diagnostic items to methods, as it would be nicer and more scalable to access them in a nicer fashion, like adding a `is_diagnostic_assoc_item(did, sym::Iterator, sym::map)` function or something similar (Suggested by `@camsteffen` [on Zulip](https://rust-lang.zulipchat.com/#narrow/stream/147480-t-compiler.2Fwg-diagnostics/topic/Diagnostic.20Item.20Naming.20Convention.3F/near/225024603))
There seems to be some different naming conventions when it comes to diagnostic items, some use UpperCamelCase (`BinaryHeap`) and some snake_case (`hashmap_type`). This PR uses UpperCamelCase for structs and traits and snake_case with the module name as a prefix for functions. Any feedback on is this welcome.
cc: rust-lang/rust-clippy#5393
r? `@Manishearth`
Add forwarder methods `Stdin::lines` and `Stdin::split`, which consume
and lock a `Stdin` handle, and forward on to the corresponding `BufRead`
methods. This should make it easier for beginners to use those iterator
constructors without explicitly dealing with locks or lifetimes.
stdio_locked: add tracking issue
Add the tracking issue number #86845 to the stability attributes for the implementation in #86799.
r? `@joshtriplett`
`@rustbot` label +A-io +C-cleanup +T-libs-api
Remove unstable `io::Cursor::remaining`
Adding `io::Cursor::remaining` in #86037 caused a conflict with the implementation of `bytes::Buf` for `io::Cursor`, leading to an error in nightly, see https://github.com/rust-lang/rust/issues/86369#issuecomment-867723485.
This fixes the error by temporarily removing the `remaining` function.
r? `@yaahc`
Stabilize `Seek::rewind()`
This stabilizes `Seek::rewind`. It seemed to fit into one of the existing tests, so I extended that test rather than adding a new one.
Closes#85149.
add owned locked stdio handles
Add stderr_locked, stdin_locked, and stdout_locked free functions
to obtain owned locked stdio handles in a single step. Also add
into_lock methods to consume a stdio handle and return an owned
lock. These methods will make it easier to use locked stdio
handles without having to deal with lifetime problems or keeping
bindings to the unlocked handles around.
Fixes#85383; enables #86412.
r? `@joshtriplett`
`@rustbot` label +A-io +C-enhancement +D-newcomer-roadblock +T-libs-api
More ErrorKinds for common errnos
From the commit message of the main commit here (as revised):
```
There are a number of IO error situations which it would be very
useful for Rust code to be able to recognise without having to resort
to OS-specific code. Taking some Unix examples, `ENOTEMPTY` and
`EXDEV` have obvious recovery strategies. Recently I was surprised to
discover that `ENOSPC` came out as `ErrorKind::Other`.
Since I am familiar with Unix I reviwed the list of errno values in
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/errno.h.html
Here, I add those that most clearly seem to be needed.
`@CraftSpider` provided information about Windows, and references, which
I have tried to take into account.
This has to be insta-stable because we can't sensibly have a different
set of ErrorKinds depending on a std feature flag.
I have *not* added these to the mapping tables for any operating
systems other than Unix and Windows. I hope that it is OK to add them
now for Unix and Windows now, and maybe add them to other OS's mapping
tables as and when someone on that OS is able to consider the
situation.
I adopted the general principle that it was usually a bad idea to map
two distinct error values to the same Rust error code. I notice that
this principle is already violated in the case of `EACCES` and
`EPERM`, which both map to `PermissionDenied`. I think this was
probably a mistake but it would be quite hard to change now, so I
don't propose to do anything about that.
However, for Windows, there are sometimes different error codes for
identical situations. Eg there are WSA* versions of some error
codes as well as ERROR_* ones. Also Windows seems to have a great
many more erorr codes. I don't know precisely what best practice
would be for Windows.
```
<strike>
```
Errno values I wasn't sure about so *haven't* included:
EMFILE ENFILE ENOBUFS ENOLCK:
These are all fairly Unix-specific resource exhaustion situations.
In practice it seemed not very likely to me that anyone would want
to handle these differently to `Other`.
ENOMEM ERANGE EDOM EOVERFLOW
Normally these don't get exposed to the Rust callers I hope. They
don't tend to come out of filesystem APIs.
EILSEQ
Hopefully Rust libraries open files in binary mode and do the
converstion in Rust. So Rust code ought not to be exposed to
EILSEQ.
EIO
The range of things that could cause this is troublesome. I found
it difficult to describe. I do think it would be useful to add this
at some point, because EIO on a filesystem operation is much more
serious than most other errors.
ENETDOWN
I wasn't sure if this was useful or, indeed, if any modern systems
use it.
ENOEXEC
It is not clear to me how a Rust program could respond to this. It
seems rather niche.
EPROTO ENETRESET ENODATA ENOMSG ENOPROTOOPT ENOSR ENOSTR ETIME
ENOTRECOVERABLE EOWNERDEAD EBADMSG EPROTONOSUPPORT EPROTOTYPE EIDRM
These are network or STREAMS related errors which I have never in
my own Unix programming found the need to do anything with. I think
someone who understands these better should be the one to try to
find good Rust names and descriptions for them.
ENOTTY ENXIO ENODEV EOPNOTSUPP ESRCH EALREADY ECANCELED ECHILD
EINPROGRESS
These are very hard to get unless you're already doing something
very Unix-specific, in which case the raw_os_error interface is
probably more suitable than relying on the Rust ErrorKind mapping.
EFAULT EBADF
These would seem to be the result of application UB.
```
</strike>
<i>(omitted errnos are discussed below, especially in https://github.com/rust-lang/rust/pull/79965#issuecomment-810468334)
Redefine `ErrorKind::Other` and stop using it in std.
This implements the idea I shared yesterday in the libs meeting when we were discussing how to handle adding new `ErrorKind`s to the standard library: This redefines `Other` to be for *user defined errors only*, and changes all uses of `Other` in the standard library to a `#[doc(hidden)]` and permanently `#[unstable]` `ErrorKind` that users can not match on. This ensures that adding `ErrorKind`s at a later point in time is not a breaking change, since the user couldn't match on these errors anyway. This way, we use the `#[non_exhaustive]` property of the enum in a more effective way.
Open questions:
- How do we check this change doesn't cause too much breakage? Will a crate run help and be enough?
- How do we ensure we don't accidentally start using `Other` again in the standard library? We don't have a `pub(not crate)` or `#[deprecated(in this crate only)]`.
cc https://github.com/rust-lang/rust/pull/79965
cc `@rust-lang/libs` `@ijackson`
r? `@dtolnay`
Add stderr_locked, stdin_locked, and stdout_locked free functions
to obtain owned locked stdio handles in a single step. Also add
into_lock methods to consume a stdio handle and return an owned
lock. These methods will make it easier to use locked stdio
handles without having to deal with lifetime problems or keeping
bindings to the unlocked handles around.
The omission of Sink: Default is causing me a slight inconvenience in
a test harness. There seems little reason for this and Empty not to
be Clone and Copy too.
I have made all three of these insta-stable, because:
AIUI Copycan only be derived, and I was not able to find any
examples of how to unstably derive it. I think it is probably not
possible.
I hunted through the git history for precedent and found
79b8ad84c8
Implement `Copy` for `IoSlice`
https://github.com/rust-lang/rust/pull/69403
which was also insta-stable.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Due to the std/alloc split, it is not possible to make
`alloc::collections::TryReserveError::AllocError` non-exhaustive without
having an unstable, doc-hidden method to construct (which negates the
benefits from `#[non_exhaustive]`.
Add `io::Cursor::{remaining, remaining_slice, is_empty}`
Tracking issue: #86369
I came across an inconvenience when answering the following [Stack Overflow](https://stackoverflow.com/questions/67831170) question.
To get the remaining slice you have to call `buff.fill_buf().unwrap()`. Which in my opinion doesn't really tell you what is returned (in the context of Cursor). To improve readability and convenience when using Cursor i propose adding the method `remaining`.
The next thing i found inconvenient (unnecessary long) was detecting if the cursor reached the end. There are a few ways this can be achieved right now:
- `buff.fill_buf().unwrap().is_empty()`
- `buff.position() >= buff.get_ref().len()`
- `buff.bytes().next().is_none()`
Which all seem a bit unintuitive, hidden in trait documentations or just a bit long for such a simple task.
Therefor i propose another method called `is_empty`, maybe with another name, since this one may leave room for interpretation on what really is empty (the underlying slice, the remaining slice or maybe the position).
Since it seemed easier to create this PR instead of an RFC i did that, if an RFC is wanted, i can close this PR and write an RFC first.
Add has_data_left() to BufRead
This is a continuation of #40747 and also addresses #40745. The problem with the previous PR was that it had "eof" in its method name. This PR uses a more descriptive method name, but I'm open to changing it.
Specialize `io::Bytes::size_hint` for more types
Improve the result of `<io::Bytes as Iterator>::size_hint` for some readers. I did not manage to specialize `SizeHint` for `io::Cursor`
Side question: would it be interesting for `io::Read` to have an optional `size_hint` method ?
Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance
Also changes the signature of `advance_slice` to accept a `&mut &mut [IoSlice]`, not returning anything. This will better match the `IoSlice::advance` function.
Updates https://github.com/rust-lang/rust/issues/62726.
To make way for a new IoSlice(Mut)::advance function that advances a
single slice.
Also changes the signature to accept a `&mut &mut [IoSlice]`, not
returning anything. This will better match the future IoSlice::advance
function.
add an example to explain std::io::Read::read returning 0 in some cases
I have always found the explanation about `Read::read` returning 0 to indicate EOF but not indefinitely, so here's more info using Linux as example. I can also add example code if necessary
Move `std::memchr` to `sys_common`
`std::memchr` is a thin abstraction over the different `memchr` implementations in `sys`, along with documentation and tests. The module is only used internally by `std`, nothing is exported externally. Code like this is exactly what the `sys_common` module is for, so this PR moves it there.
Override `clone_from` for some types
Override `clone_from` method of the `Clone` trait for:
- `cell::RefCell`
- `cmp::Reverse`
- `io::Cursor`
- `mem::ManuallyDrop`
This can bring performance improvements.
Optimize for the common case where the input write size is less than the
buffer size. This slightly increases the cost for pathological write
patterns that commonly fill the buffer exactly, but if a client is doing
that frequently, they're already paying the cost of frequent flushing,
etc., so the cost is of this optimization to them is relatively small.
We use a Vec as our internal, constant-sized buffer, but the overhead of
using methods like `extend_from_slice` can be enormous, likely because
they don't get inlined, because `Vec` has to repeat bounds checks that
we've already done, and because it makes considerations for things like
reallocating, even though they should never happen.
Ensure that `write` and `write_all` can be inlined and that their
commonly executed fast paths can be as short as possible.
`write_vectored` would likely benefit from the same optimization, but I
omitted it because its implementation is more complex, and I don't have
a benchmark on hand to guide its optimization.
Stabilize `bufreader_seek_relative`
This PR marks `BufReader::seek_relative` as stable - the associated issue, #31100, has passed the final comment period without any issues, and from what I understand, the only thing left to stabilize this is to submit a PR marking the method as stable.
Closes#31100.
Rename `#[doc(spotlight)]` to `#[doc(notable_trait)]`
Fixes#80936.
"spotlight" is not a very specific or self-explaining name.
Additionally, the dialog that it triggers is called "Notable traits".
So, "notable trait" is a better name.
* Rename `#[doc(spotlight)]` to `#[doc(notable_trait)]`
* Rename `#![feature(doc_spotlight)]` to `#![feature(doc_notable_trait)]`
* Update documentation
* Improve documentation
r? `@Manishearth`
Derive Debug for io::Chain instead of manually implementing it.
This derives Debug for io::Chain instead of manually implementing it.
The manual implementation has the same bounds, so I don't think there's any reason for a manual implementation. The names used in the derive implementation are even nicer (`first`/`second`) than the manual implementation (`t`/`u`), and include the `done_first` field too.
The manual implementation has the same bounds, so I don't think there's
any reason for a manual implementation. The names used in the derive
implementation are even nicer (`first`/`second`) than the manual
implementation (`t`/`u`), and include the `done_first` field too.
Add internal io::Error::new_const to avoid allocations.
This makes it possible to have a io::Error containing a message with zero allocations, and uses that everywhere to avoid the *three* allocations involved in `io::Error::new(kind, "message")`.
The function signature isn't perfect, because it needs a reference to the `&str`. So for now, this is just a `pub(crate)` function. Later, we'll be able to use `fn new_const<MSG: &'static str>(kind: ErrorKind)` to make that a bit better. (Then we'll also be able to use some ZST trickery if that would result in more efficient code.)
See https://github.com/rust-lang/rust/issues/83352
Clarify docs for Read::read's return value
Right now the docs for `Read::read`'s return value are phrased in a way that makes it easy for the reader to assume that the return value is never larger than the passed buffer. This PR clarifies that this is a requirement for implementations of the trait, but that callers have to expect a buggy yet safe implementation failing to do so, especially if unchecked accesses to the buffer are done afterwards.
I fell into this trap recently, and when I noticed, I looked at the docs again and had the feeling that I might not have been the first one to miss this.
The same issue of trusting the return value of `read` was also present in std itself for about 2.5 years and only fixed recently, see #80895.
I hope that clarifying the docs might help others to avoid this issue.
"spotlight" is not a very specific or self-explaining name.
Additionally, the dialog that it triggers is called "Notable traits".
So, "notable trait" is a better name.
* Rename `#[doc(spotlight)]` to `#[doc(notable_trait)]`
* Rename `#![feature(doc_spotlight)]` to `#![feature(doc_notable_trait)]`
* Update documentation
* Improve documentation
As discussed in the issue tracker for the wg-allocators working group[1], updating this implementation for allocator support was most likely just forgotten in the original PR.
[1]: https://github.com/rust-lang/wg-allocators/issues/86
Avoid unnecessary Vec construction in BufReader
As mentioned in #80460, creating a `Vec` and calling `Vec::into_boxed_slice()` emits unnecessary calls to `realloc()` and `free()`. Updated the code to use `Box::new_uninit_slice()` to create a boxed slice directly. I think this also makes it more explicit that the initial contents of the buffer are uninitialized.
r? ``@m-ou-se``
Improved IO Bytes Size Hint
After trying to implement better `size_hint()` return values for `File` in [this PR](https://github.com/rust-lang/rust/pull/81044) and changing to implementing it for `BufReader` in [this PR](https://github.com/rust-lang/rust/pull/81052), I have arrived at this implementation that provides tighter bounds for the `Bytes` iterator of various readers including `BufReader`, `Empty`, and `Chain`.
Unfortunately, for `BufReader`, the size_hint only improves after calling `fill_buffer` due to it using the contents of the buffer for the hint. Nevertheless, the the tighter bounds should result in better pre-allocation of space to handle the contents of the `Bytes` iterator.
Closes#81052
Convert primitives in the standard library to intra-doc links
Blocked on https://github.com/rust-lang/rust/pull/80181. I forgot that this needs to wait for the beta bump so the standard library can be documented with `doc --stage 0`.
Notably I didn't convert `core::slice` because it's like 50 links and I got scared 😨
Add missing "see its documentation for more" stdio
StdoutLock and StderrLock does not have example, it would be better
to leave "see its documentation for more" like iter docs.
Almost all safety comments are of the form `// SAFETY:`,
so normalize the rest and fix a few of them that should
have been a `/// # Safety` section instead.
Furthermore, make `tidy` only allow the uppercase form. While
currently `tidy` only checks `core`, it is a good idea to prevent
`core` from drifting to non-uppercase comments, so that later
we can start checking `alloc` etc. too.
Signed-off-by: Miguel Ojeda <ojeda@kernel.org>
BufWriter: Provide into_raw_parts
If something goes wrong, one might want to unpeel the layers of nested
Writers to perform recovery actions on the underlying writer, or reuse
its resources.
`into_inner` can be used for this when the inner writer is still
working. But when the inner writer is broken, and returning errors,
`into_inner` simply gives you the error from flush, and the same
`Bufwriter` back again.
Here I provide the necessary function, which I have chosen to call
`into_raw_parts`.
I had to do something with `panicked`. Returning it to the caller as
a boolean seemed rather bare. Throwing the buffered data away in this
situation also seems unfriendly: maybe the programmer knows something
about the underlying writer and can recover somehow.
So I went for a custom Error. This may be overkill, but it does have
the nice property that a caller who actually wants to look at the
buffered data, rather than simply extracting the inner writer, will be
told by the type system if they forget to handle the panicked case.
If a caller doesn't need the buffer, it can just be discarded. That
WriterPanicked is a newtype around Vec<u8> means that hopefully the
layouts of the Ok and Err variants can be very similar, with just a
boolean discriminant. So this custom error type should compile down
to nearly no code.
*If this general idea is felt appropriate, I will open a tracking issue, etc.*
Fix handling of malicious Readers in read_to_end
A malicious `Read` impl could return overly large values from `read`, which would result in the guard's drop impl setting the buffer's length to greater than its capacity! ~~To fix this, the drop impl now uses the safe `truncate` function instead of `set_len` which ensures that this will not happen. The result of calling the function will be nonsensical, but that's fine given the contract violation of the `Read` impl.~~
~~The `Guard` type is also used by `append_to_string` which does not pass untrusted values into the length field, so I've copied the guard type into each function and only modified the one used by `read_to_end`. We could just keep a single one and modify it, but it seems a bit cleaner to keep the guard code close to the functions and related specifically to them.~~
To fix this, we now assert that the returned length is not larger than the buffer passed to the method.
For reference, this bug has been present for ~2.5 years since 1.20: ecbb896b9e.
Closes#80894.
Add a `std::io::read_to_string` function
I recognize that you're usually supposed to open an issue first, but the
implementation is very small so it's okay if this is closed and it was 'wasted
work' :)
-----
The equivalent of `std::fs::read_to_string`, but generalized to all
`Read` impls.
As the documentation on `std::io::read_to_string` says, the advantage of
this function is that it means you don't have to create a variable first
and it provides more type safety since you can only get the buffer out
if there were no errors. If you use `Read::read_to_string`, you have to
remember to check whether the read succeeded because otherwise your
buffer will be empty.
It's friendlier to newcomers and better in most cases to use an explicit
return value instead of an out parameter.
The heading style for `std::prelude` is to be consistent with the
headings for `std` and `core`: `# The Rust Standard Library` and
`# The Rust Core Library`, respectively.
The equivalent of `std::fs::read_to_string`, but generalized to all
`Read` impls.
As the documentation on `std::io::read_to_string` says, the advantage of
this function is that it means you don't have to create a variable first
and it provides more type safety since you can only get the buffer out
if there were no errors. If you use `Read::read_to_string`, you have to
remember to check whether the read succeeded because otherwise your
buffer will be empty.
It's friendlier to newcomers and better in most cases to use an explicit
return value instead of an out parameter.
Enforce no-move rule of ReentrantMutex using Pin and fix UB in stdio
A `sys_common::ReentrantMutex` may not be moved after initializing it with `.init()`. This was not enforced, but only stated as a requirement in the comments on the unsafe functions. This change enforces this no-moving rule using `Pin`, by changing `&self` to a `Pin` in the `init()` and `lock()` functions.
This uncovered a bug I introduced in #77154: stdio.rs (the only user of ReentrantMutex) called `init()` on its ReentrantMutexes while constructing them in the intializer of `SyncOnceCell::get_or_init`, which would move them afterwards. Interestingly, the ReentrantMutex unit tests already had the same bug, so this invalid usage has been tested on all (CI-tested) platforms for a long time. Apparently this doesn't break badly on any of the major platforms, but it does break the rules.\*
To be able to keep using SyncOnceCell, this adds a `SyncOnceCell::get_or_init_pin` function, which makes it possible to work with pinned values inside a (pinned) SyncOnceCell. Whether this function should be public or not and what its exact behaviour and interface should be if it would be public is something I'd like to leave for a separate issue or PR. In this PR, this function is internal-only and marked with `pub(crate)`.
\* Note: That bug is now included in 1.48, while this patch can only make it to ~~1.49~~ 1.50. We should consider the implications of 1.48 shipping with a wrong usage of `pthread_mutex_t` / `CRITICAL_SECTION` / .. which technically invokes UB according to their specification. The risk is very low, considering the objects are not 'used' (locked) before the move, and the ReentrantMutex unit tests have verified this works fine in practice.
Edit: This has been backported and included in 1.48. And soon 1.49 too.
---
In future changes, I want to push this usage of Pin further inside `sys` instead of only `sys_common`, and apply it to all 'unmovable' objects there (`Mutex`, `Condvar`, `RwLock`). Also, while `sys_common`'s mutexes and condvars are already taken care of by #77147 and #77648, its `RwLock` should still be made movable or get pinned.
Use is_write_vectored to optimize the write_vectored implementation for BufWriter
In case when the underlying writer does not have an efficient implementation `write_vectored`, the present implementation of
`write_vectored` for `BufWriter` may still forward vectored writes directly to the writer depending on the total length of the data. This misses the advantage of buffering, as the actually written slice may be small.
Provide an alternative code path for the non-vectored case, where the slices passed to `BufWriter` are coalesced in the buffer before being flushed to the underlying writer with plain `write` calls. The buffer is only bypassed if an individual slice's length is at least as large as the buffer.
Remove a FIXME comment referring to #72919 as the issue has been closed with an explanation provided.
The code in io::stdio before this change misused the ReentrantMutexes,
by calling init() on them and moving them afterwards. Now that
ReentrantMutex requires Pin for init(), this mistake is no longer easy
to make.
In particular, IntoIneerError only currently provides .error() which
returns a reference, not an owned value. This is not helpful and
means that a caller of BufWriter::into_inner cannot acquire an owned
io::Error which seems quite wrong.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
If something goes wrong, one might want to unpeel the layers of nested
Writers to perform recovery actions on the underlying writer, or reuse
its resources.
`into_inner` can be used for this when the inner writer is still
working. But when the inner writer is broken, and returning errors,
`into_inner` simply gives you the error from flush, and the same
`Bufwriter` back again.
Here I provide the necessary function, which I have chosen to call
`into_raw_parts`.
I had to do something with `panicked`. Returning it to the caller as
a boolean seemed rather bare. Throwing the buffered data away in this
situation also seems unfriendly: maybe the programmer knows something
about the underlying writer and can recover somehow.
So I went for a custom Error. This may be overkill, but it does have
the nice property that a caller who actually wants to look at the
buffered data, rather than simply extracting the inner writer, will be
told by the type system if they forget to handle the panicked case.
If a caller doesn't need the buffer, it can just be discarded. That
WriterPanicked is a newtype around Vec<u8> means that hopefully the
layouts of the Ok and Err variants can be very similar, with just a
boolean discriminant. So this custom error type should compile down
to nearly no code.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Do what write does and optimize for the most likely case:
slices are much smaller than the buffer. If a slice does not fit
completely in the remaining capacity of the buffer, it is left out
rather than buffered partially. Special treatment is only left for
oversized slices that are written directly to the underlying writer.
Now that BufWriter always claims to support vectored writes,
look through it at the wrapped writer to decide whether to
use vectored writes for LineWriter.
If the underlying writer does not support efficient vectored output,
do it differently: always try to coalesce the slices in the buffer
until one comes that does not fit entirely. Flush the buffer before
the first slice if needed.
These referred to a “`Write`er”—extra *e*. Presumably a copy-paste
holdover from “`Read`er”.
Test Plan:
Running ``git grep '`\?[Ww]rite`\?er'`` no longer finds any results.
wchargin-branch: io-write-docs
Simplify output capturing
This is a sequence of incremental improvements to the unstable/internal `set_panic` and `set_print` mechanism used by the `test` crate:
1. Remove the `LocalOutput` trait and use `Arc<Mutex<dyn Write>>` instead of `Box<dyn LocalOutput>`. In practice, all implementations of `LocalOutput` were just `Arc<Mutex<..>>`. This simplifies some logic and removes all custom `Sink` implementations such as `library/test/src/helpers/sink.rs`. Also removes a layer of indirection, as the outermost `Box` is now gone. It also means that locking now happens per `write_fmt`, not per individual `write` within. (So `"{} {}\n"` now results in one `lock()`, not four or more.)
2. Since in all cases the `dyn Write`s were just `Vec<u8>`s, replace the type with `Arc<Mutex<Vec<u8>>>`. This simplifies things more, as error handling and flushing can be removed now. This also removes the hack needed in the default panic handler to make this work with `::realstd`, as (unlike `Write`) `Vec<u8>` is from `alloc`, not `std`.
3. Replace the `RefCell`s by regular `Cell`s. The `RefCell`s were mostly used as `mem::replace(&mut *cell.borrow_mut(), something)`, which is just `Cell::replace`. This removes an unecessary bookkeeping and makes the code a bit easier to read.
4. Merge `set_panic` and `set_print` into a single `set_output_capture`. Neither the test crate nor rustc (the only users of this feature) have a use for using these separately. Merging them simplifies things even more. This uses a new function name and feature name, to make it clearer this is internal and not supposed to be used by other crates.
Might be easier to review per commit.
specialize io::copy to use copy_file_range, splice or sendfile
Fixes#74426.
Also covers #60689 but only as an optimization instead of an official API.
The specialization only covers std-owned structs so it should avoid the problems with #71091
Currently linux-only but it should be generalizable to other unix systems that have sendfile/sosplice and similar.
There is a bit of optimization potential around the syscall count. Right now it may end up doing more syscalls than the naive copy loop when doing short (<8KiB) copies between file descriptors.
The test case executes the following:
```
[pid 103776] statx(3, "", AT_STATX_SYNC_AS_STAT|AT_EMPTY_PATH, STATX_ALL, {stx_mask=STATX_ALL|STATX_MNT_ID, stx_attributes=0, stx_mode=S_IFREG|0644, stx_size=17, ...}) = 0
[pid 103776] write(4, "wxyz", 4) = 4
[pid 103776] write(4, "iklmn", 5) = 5
[pid 103776] copy_file_range(3, NULL, 4, NULL, 5, 0) = 5
```
0-1 `stat` calls to identify the source file type. 0 if the type can be inferred from the struct from which the FD was extracted
𝖬 `write` to drain the `BufReader`/`BufWriter` wrappers. only happen when buffers are present. 𝖬 ≾ number of wrappers present. If there is a write buffer it may absorb the read buffer contents first so only result in a single write. Vectored writes would also be an option but that would require more invasive changes to `BufWriter`.
𝖭 `copy_file_range`/`splice`/`sendfile` until file size, EOF or the byte limit from `Take` is reached. This should generally be *much* more efficient than the read-write loop and also have other benefits such as DMA offload or extent sharing.
## Benchmarks
```
OLD
test io::tests::bench_file_to_file_copy ... bench: 21,002 ns/iter (+/- 750) = 6240 MB/s [ext4]
test io::tests::bench_file_to_file_copy ... bench: 35,704 ns/iter (+/- 1,108) = 3671 MB/s [btrfs]
test io::tests::bench_file_to_socket_copy ... bench: 57,002 ns/iter (+/- 4,205) = 2299 MB/s
test io::tests::bench_socket_pipe_socket_copy ... bench: 142,640 ns/iter (+/- 77,851) = 918 MB/s
NEW
test io::tests::bench_file_to_file_copy ... bench: 14,745 ns/iter (+/- 519) = 8889 MB/s [ext4]
test io::tests::bench_file_to_file_copy ... bench: 6,128 ns/iter (+/- 227) = 21389 MB/s [btrfs]
test io::tests::bench_file_to_socket_copy ... bench: 13,767 ns/iter (+/- 3,767) = 9520 MB/s
test io::tests::bench_socket_pipe_socket_copy ... bench: 26,471 ns/iter (+/- 6,412) = 4951 MB/s
```
It was only ever used with Vec<u8> anyway. This simplifies some things.
- It no longer needs to be flushed, because that's a no-op anyway for
a Vec<u8>.
- Writing to a Vec<u8> never fails.
- No #[cfg(test)] code is needed anymore to use `realstd` instead of
`std`, because Vec comes from alloc, not std (like Write).
Capture output from threads spawned in tests
This is revival of #75172.
Original text:
> Fixes#42474.
>
> r? `@dtolnay` since you expressed interest in this, but feel free to redirect if you aren't the right person anymore.
---
Closes#75172.
Refactor io/buffered.rs into submodules
This pull request splits `BufWriter`, `BufReader`, `LineWriter`, and `LineWriterShim` (along with their associated tests) into separate submodules. It contains no functional changes. This change is being made in anticipation of adding another type of buffered writer which can be switched between line- and block-buffering mode.
Part of a series of pull requests resolving #60673.
The thread local LOCAL_STDOUT and LOCAL_STDERR are only used by the test
crate to capture output from tests when running them in the same process
in differen threads. However, every program will check these variables
on every print, even outside of testing.
This involves allocating a thread local key, and registering a thread
local destructor. This can be somewhat expensive.
This change keeps a global flag (LOCAL_STREAMS) which will be set to
true when either of these local streams is used. (So, effectively only
in test and benchmark runs.) When this flag is off, these thread locals
are not even looked at and therefore will not be initialized on the
first output on every thread, which also means no thread local
destructors will be registered.
Remove std::io::lazy::Lazy in favour of SyncOnceCell
The (internal) std::io::lazy::Lazy was used to lazily initialize the stdout and stdin buffers (and mutexes). It uses atexit() to register a destructor to flush the streams on exit, and mark the streams as 'closed'. Using the stream afterwards would result in a panic.
Stdout uses a LineWriter which contains a BufWriter that will flush the buffer on drop. This one is important to be executed during shutdown, to make sure no buffered output is lost. It also forbids access to stdout afterwards, since the buffer is already flushed and gone.
Stdin uses a BufReader, which does not implement Drop. It simply forgets any previously read data that was not read from the buffer yet. This means that in the case of stdin, the atexit() function's only effect is making stdin inaccessible to the program, such that later accesses result in a panic. This is uncessary, as it'd have been safe to access stdin during shutdown of the program.
---
This change removes the entire io::lazy module in favour of SyncOnceCell. SyncOnceCell's fast path is much faster (a single atomic operation) than locking a sys_common::Mutex on every access like Lazy did.
However, SyncOnceCell does not use atexit() to drop the contained object during shutdown.
As noted above, this is not a problem for stdin. It simply means stdin is now usable during shutdown.
The atexit() call for stdout is moved to the stdio module. Unlike the now-removed Lazy struct, SyncOnceCell does not have a 'gone and unusable' state that panics. Instead of adding this again, this simply replaces the buffer with one with zero capacity. This effectively flushes the old buffer *and* makes any writes afterwards pass through directly without touching a buffer, making print!() available during shutdown without panicking.
---
In addition, because the contents of the SyncOnceCell are no longer dropped, we can now use `&'static` instead of `Arc` in `Stdout` and `Stdin`. This also saves two levels of indirection in `stdin()` and `stdout()`, since Lazy effectively stored a `Box<Arc<T>>`, and SyncOnceCell stores the `T` directly.
The (internal) std::io::lazy::Lazy was used to lazily initialize the
stdout and stdin buffers (and mutexes). It uses atexit() to register a
destructor to flush the streams on exit, and mark the streams as
'closed'. Using the stream afterwards would result in a panic.
Stdout uses a LineWriter which contains a BufWriter that will flush the
buffer on drop. This one is important to be executed during shutdown,
to make sure no buffered output is lost. It also forbids access to
stdout afterwards, since the buffer is already flushed and gone.
Stdin uses a BufReader, which does not implement Drop. It simply forgets
any previously read data that was not read from the buffer yet. This
means that in the case of stdin, the atexit() function's only effect is
making stdin inaccessible to the program, such that later accesses
result in a panic. This is uncessary, as it'd have been safe to access
stdin during shutdown of the program.
---
This change removes the entire io::lazy module in favour of
SyncOnceCell. SyncOnceCell's fast path is much faster (a single atomic
operation) than locking a sys_common::Mutex on every access like Lazy
did.
However, SyncOnceCell does not use atexit() to drop the contained object
during shutdown.
As noted above, this is not a problem for stdin. It simply means stdin
is now usable during shutdown.
The atexit() call for stdout is moved to the stdio module. Unlike the
now-removed Lazy struct, SyncOnceCell does not have a 'gone and
unusable' state that panics. Instead of adding this again, this simply
replaces the buffer with one with zero capacity. This effectively
flushes the old buffer *and* makes any writes afterwards pass through
directly without touching a buffer, making print!() available during
shutdown without panicking.
Implement Seek::stream_position() for BufReader
Optimization over `BufReader::seek()` for getting the current position without flushing the internal buffer.
Related to #31100. Based on the code in #70577.
rename get_{ref, mut} to assume_init_{ref,mut} in Maybeuninit
References #63568
Rework with comments addressed from #66174
Have replaced most of the occurrences I've found, hopefully didn't miss out anything
r? @RalfJung
(thanks @danielhenrymantilla for the initial work on this)
Substantial refactor to the design of LineWriter
# Preamble
This is the first in a series of pull requests designed to move forward with https://github.com/rust-lang/rust/issues/60673 (and the related [5 year old FIXME](ea7181b5f7/src/libstd/io/stdio.rs (L459-L461))), which calls for an update to `Stdout` such that it can be block-buffered rather than line-buffered under certain circumstances (such as a `tty`, or a user setting the mode with a function call). This pull request refactors the logic `LineWriter` into a `LineWriterShim`, which operates on a `BufWriter` by mutable reference, such that it is easy to invoke the line-writing logic on an existing `BufWriter` without having to construct a new `LineWriter`.
Additionally, fixes#72721
## A note on flushing
Because the word **flush** tends to be pretty overloaded in this discussion, I'm going to use the word **unbuffered** to refer to a `BufWriter` sending its data to the wrapped writer via `write`, without calling `flush` on it, and I'll be using **flushed** when referring to sending data via flush, which recursively writes the data all the way to the final sink.
For example, given a `T = BufWriter<BufWriter<File>>`, saying that `T` **unbuffers** its data means that it is sent to the inner `BufWriter`, but not necessarily to the `File`, whereas saying that `T` **flushes** its data means that causes it (via `Write::flush`) to be delivered all the way to `File`.
# Goals
Once it became clear (for reasons described below) that the best way to approach this would involve refactoring `LineWriter` to work more directly on `BufWriter`'s internals, I established the following design goals for the refactor:
- Do not duplicate logic with `BufWriter`. It's great at buffering and then unbuffering data, so use the existing logic as much as possible.
- Minimize superfluous copying of data into `BufWriter`'s buffer.
- Eliminate calls to `BufWriter::flush` and instead do the same thing as `BufWriter::write`, which is to only write to the wrapped writer (rather than flushing all the way down to the final data sink).
- Uphold the "at-most 1 write of new data" convention of `Write::write`
- Minimize or eliminate dropping errors (that is, eliminate the parts of the old design that threw away errors because `write` *must* report if any bytes were written)
- As much as possible, attempt to fully flush completed lines, and *not* flush partial lines. One of the advantages of this design is that, so long as we don't encounter lines larger than the `BufWriter`'s capacity, partial lines will never be unbuffered, while completed lines will *always* be unbuffered (with subsequent calls to `LineWriter::write` retrying failed writes before processing new data.
# Design
There are two major & related parts of the design.
First, a new internal stuct, `LineWriterShim`, is added. This struct implements all of the actual logic of line-writing in a `Write` implementation, but it only operates on an `&mut BufWriter`. This means that this shim can be constructed on-the-fly to apply line writing logic to an existing `BufWriter`. This is in fact how `LineWriter` has been updated to operate, and it is also how `Stdout` is being updated in my [development branch](https://github.com/Lucretiel/rust/tree/stdout-block-buffer) to switch which mode it wants to use at runtime.
[An example of how this looks in practice](f24f272df6/src/libstd/io/stdio.rs (L479-L484)
)
The second major part of the design that the line-buffering logic, implemented in `LineWriterShim`, has been updated to work slightly more directly on the internals of `BufWriter`. Mostly it makes us of the public interface—particularly `buffer()` and `get_mut()`—but it also controls the flushing of the buffer with `flush_buf` rather than `flush`, and it writes to the buffer infallibly with a new `write_to_buffer` method. This has several advantages:
- Data no longer has to round trip through the `BufWriter`'s buffer. If the user provides a complete line, that line is written directly to the inner writer (after ensuring the existing buffer is flushed).
- The conventional contract of `write`—that at-most 1 attempt to write new data is made—is much more cleanly upheld, because we don't have to perform fallible flushes and perform semi-complicated logic of trying to pretend errors at different stages didn't happen. Instead, after attempting to write lines directly to the buffer, we can infallibly add trailing data to the buffer without allowing any attempts to continue writing it to the `inner` writer.
- Perhaps most importantly, `LineWriter` *no longer performs a full flush on every line.* This makes its behavior much more consistent with `BufWriter`, which unbuffers data to its inner writer, without trying to flush it all the way to the final device. Previously, `LineWriter` had no choice but to use `flush` to ensure that the lines were unbuffered, but by writing directly to `inner` via `get_mut()` (when appropriate), we can use a more correct behavior.
## New(ish) line buffering logic
The logic for line writing has been cleaned up, as described above. It now follows this algorithm for `write`, with minor adjustments for `write_all` and `write_vectored`:
- Does our input data contain a newline?
- If no:
- simply use the regular `BufWriter::write` to write it; this will append it to the buffer and/or flush it as necessary based on how full the buffer is and how much input data there is.
- additionally, if the current buffer ends with `'\n'`, attempt to immediately flush it with `flush_buf` before calling `BufWriter::write` This reproduces the old `needs_flush` behavior and ensures completed lines are flushed as soon as possible. The reason we only check if the buffer *ends* with `'\n'` is discussed later.
- If yes:
- First, `flush_buf`
- Then use `bufwriter.get_mut().write()` to write the input data directly to the underlying writer, up to the last newline. Make at most one attempt at this.
- If it errors, return the error
- If it succeeds with a full write, add the remaining data (between the last newline and the end of the input) to the buffer. In order to uphold the "at-most 1 attempt to write new data" convention, no attempts are made to write this data to the inner writer (though obviously a subsequent write may immediately flush it, e.g., if it totally filled the buffer's capacity.
- If it only partially succeeds, buffer the data only up to the last newline. We do this to try to avoid writing partial lines to the inner writer where possible (that is, whenever the lines are shorter than the total buffer capacity).
While it was not my intention for this behavior to diverge from this existing `LineWriter` algorithm, this updated design emerged very naturally once `LineWriter` wasn't burdened with having to only operate via `BufWriter::flush`. There essentially two main changes to observable behavior:
- `flush` is no longer used to unbuffer lines. The are only written to the writer wrapped by `LineWriter`; this inner writer might do its own buffering. This change makes `LineWriter` consistent with the behavior of `BufWriter`. This is probably the most obvious user-visible change; it's the one I most expect to provoke issue reports, if any are provoked.
- Unless a line exceeds the capacity of the buffer, partial lines are not unbuffered (without the user manually calling flush). This is a less surprising behavior, and is enabled because `LineWriter` now has more precise control of what data is buffered and when it is unbuffered. I'd be surprised if anyone is relying on `LineWriter` unbuffering or flushing *partial* lines that are shorter than the capacity, so I'm not worried about this one.
None of these changes are inconsistent with any published documentation of `LineWriter`. Nonetheless, like all changes with user-facing behavior changes, this design will obviously have to be very carefully scrutinized.
# Alternative designs and design rationalle
The initial goal of this project was to provide a way for the `LineWriter` logic to be operable directly on a `BufWriter`, so that the updated `Stdout` doesn't need to do something convoluted like `enum { BufWriter, LineWriter }` (which ends up being ~~impossible~~ difficult to transition between states after being constructed). The design went through several iterations before arriving at the current draft.
The major first version simply involved adding methods like `write_line_buffered` to `BufWriter`; these would contain the actual logic of line-buffered writing, and would additionally have the advantages (described above) of operating directly on the internals of `BufWriter`. The idea was that `LineWriter` would simply call these methods, and the updated `Stdout` would use either `BufWriter::write` or `BufWriter::write_line_buffered`, depending on what mode it was in.
The major issue with this design is that it loses the ability to take advantage of the `io::Write` trait, which provides several useful default implementations of the various io methods, such as `write_fmt` and `write_all`, just using the core methods. For this reason, the `write_line_buffered` design was retained, but moved into a separate struct called `LineWriterShim` which operates on an `&mut LineWriter`. As part of this move, the logic was lightly retooled to not touch the innards of `BufWriter` directly, but instead to make use of the unexported helper methods like `flush_buf`.
The other design evolutions were mostly related to answering questions like "how much data should be buffered", "how should partial line writes be handled", etc. As much as possible I tried to answer these by emulating the current `LineWriter` logic (which, for example, retries partial line writes on subsequent calls to `write`) while still meeting the refactor design goals.
# Next steps
~Currently, this design fails a few `LineWriter` tests, mostly because they expect `LineWriter` to *fully* flush its content. There are also some changes to the way that `LineWriter` buffers data *after* writing completed lines, aimed at ensuring that partial lines are not unbuffered prematurely. I want to make sure I fully understand the intent behind these tests before I either update the test or update this design so that they pass.~
However, in the meantime I wanted to get this published so that feedback could start to accumulate on it. There's a lot of errata around how I arrived at this design that didn't really fit in this overlong document, so please ask questions about anything that confusing or unclear and hopefully I can explain more of the rationale that led to it.
# Test updates
This design required some tests to be updated; I've research the intent behind these tests (mostly via `git blame`) and updated them appropriately. Those changes are cataloged here.
- `test_line_buffer_fail_flush`: This test was added as a regression test for #32085, and is intended to assure that an errors from `flush` aren't propagated when preceded by a successful `write`. Because type of issue is no longer possible, because `write` calls `buffer.get_mut().write()` instead of `buffer.write(); buffer.flush();`, I'm simply removing this test entirely. Other, similar error invariants related to errors during write-retrying are handled in other test cases.
- `erroneous_flush_retried`: This test was added as a regression test for #37807, and was intended to ensure that flush-retrying (via `needs_flush`) and error-ignoring were being handled correctly (ironically, this issue was caused by the flush-error-ignoring, above). Half of that issue is not possible by design with this refactor, because we no longer make fallible i/o calls that might produce errors we have to ignore after unbuffering lines. The `should_flush` behavior is captured by checking for a trailing newline in the `LineWriter` buffer; this test now checks that behavior.
- `line_vectored`: changes here were pretty minor, mostly related to when partial lines are or aren't written. The old implementation of `write_vectored` used very complicated logic to precisely determine the location of the last newline and precisely write up to that point; this required doing several consecutive fallible writes, with all the complex error handling or ignoring issues that come with it. The updated design does at-most one write of a subset of total buffers (that is, it doesn't split in the middle of a buffer), even if that means writing partial lines. One of the major advantages of the new design is that the underlying vectored write operation on the device can be taken advantage of, even with small writes, so long as they include a newline; previously these were unconditionally buffered then written.
- `line_vectored_partial_and_errors`: Pretty similiar to `line_vectored`, above; this test is for basic error recovery in `write_vectored` for vectored writes. As previously discussed, the mocked behavior being tested for (errors ignored under certain circumstances) no occurs, so I've simplified the test while doing my best to retain its spirit.
