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).
