Ensure non-empty buffers for large vectored I/O

[thaliaarchi]

readv and writev are constrained by a platform-specific upper bound on the number of buffers which can be passed. Currently, read_vectored and write_vectored implementations simply truncate to this limit when larger. However, when the only non-empty buffers are at indices above this limit, they will erroneously return Ok(0).

Instead, slice the buffers starting at the first non-empty buffer. This trades a conditional move for a branch, so it's barely a penalty in the common case.

The new method limit_slices on IoSlice and IoSliceMut may be generally useful to users like advance_slices is, but I have left it as pub(crate) for now.

[rustbot]

r? @jhpratt

rustbot has assigned @jhpratt.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[thaliaarchi]

Since this involves what we guarantee for these methods, I want to make sure T-libs-api sees it too:

r? libs-api

The original question that led to this patch was whether these APIs are guaranteed to return Ok(0) iff the buffers are all empty or EOF is reached. As it stands, the implementation breaks that guarantee. However, it is stated on read and write, but not the vectored variants. Users do not have access to max_iov(), so they cannot do this logic themselves and it would make no sense to push the responsibility to them.

[thaliaarchi]

I've added a test for this case. The second assertion fails before this patch.

[bors]

☔ The latest upstream changes (presumably #139101) made this pull request unmergeable. Please resolve the merge conflicts.

[GrayJack]

This test is falling for me on macOS on the first assertion, after a few hours of investigation, I came to the conclusion that this is happening because the new method (limit_slices) will return a empty slice in the case of all IoSlice are also empty. That means that writev is invocked with iovcount of zero, which, if following POSIX, is disallowed and should return -1 with errno = EINVAL. That is the exact behavior I'm getting.

If I'm correct, and if returning ok-zero if &[IoSlice] is empty is the intended behavior for Rust, Rust then should maybe special case this? Maybe not calling the syscall and returning Ok(0) directly.

Another option I though of is maybe letting one empty IoSlice in the bufs if all IoSlices are empty, calling writev with iovcnt of 1 that is an empty iovec.

POSIX reference I read: https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html

EDIT: Looks like readv also is specified to same behavior if iovcnt is zero

[thaliaarchi]

Thanks for such a helpful diagnosis! Turns out I also encounter that same error locally for the first assertion, also on macOS. With this patch, it fixes it. I decided to slice it to one empty buffer and still invoke writev, because there aren't other cases currently that skip the writev and this is a very rare case.

Also, what you bring up illustrates a larger, preexisting issue. Should we allow zero-length buffer slices or expose the platform's behavior? I'd argue, given that Rust smooths over the case of too-large buffer slices, that this should be similarly handled. We could even handle both cases together by branching on if bufs.len().wrapping_sub(1) > IOV_MAX - 1.

[GrayJack]

I'd argue that Rust should smooth things over whenever possible if it directly affects an public API that is meant to work in a platform independent way, like File is. I expect platform-dependent behavior overall and platform-dependent things on specialized modules (like std::os::* and std::arch::* modules)

[thaliaarchi]

I implemented the empty slice case, but I'm not satisfied with the approach, so it's a separate commit for now.

I needed to implement it as a macro, because I couldn't figure out how to get this sort of pattern to compile. When given an empty slice, it needs to conjure up a &[IoSlice<'_>] or &mut [IoSliceMut<'_>] which contains a single empty buffer. Since it cannot be modified, I'd think it would be fine to reference a static (though Miri might complain about multiple mutable references to the same location). To circumvent that, I instead have the outer function return Ok(0) in this case (hence the need for a macro).

use std::io::{IoSlice, IoSliceMut};

fn replace<'a>(bufs: &mut &[IoSlice<'a>], bufs_mut: &mut &mut [IoSliceMut<'a>]) {
    *bufs = &[IoSlice::new(&[])];
    *bufs_mut = &mut [IoSliceMut::new(&mut [])];
}

error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:5:14
  |
4 | fn replace<'a>(bufs: &mut &[IoSlice<'a>], bufs_mut: &mut &mut [IoSliceMut<'a>]) {
  |                           - let's call the lifetime of this reference `'1`
5 |     *bufs = &[IoSlice::new(&[])];
  |     ---------^^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
  |     |        |
  |     |        creates a temporary value which is freed while still in use
  |     assignment requires that borrow lasts for `'1`

error[E0716]: temporary value dropped while borrowed
 --> src/main.rs:6:22
  |
4 | fn replace<'a>(bufs: &mut &[IoSlice<'a>], bufs_mut: &mut &mut [IoSliceMut<'a>]) {
  |                                                          - let's call the lifetime of this reference `'2`
5 |     *bufs = &[IoSlice::new(&[])];
6 |     *bufs_mut = &mut [IoSliceMut::new(&mut [])];
  |     -----------------^^^^^^^^^^^^^^^^^^^^^^^^^^- temporary value is freed at the end of this statement
  |     |                |
  |     |                creates a temporary value which is freed while still in use
  |     assignment requires that borrow lasts for `'2`

For more information about this error, try `rustc --explain E0716`.

[thaliaarchi]

I suspect not all platforms have the restriction that !bufs.is_empty() like POSIX. Perhaps Windows or SOLID don't care. In that case, the limit_slices logic should be moved to each platform and made more specific to their requirements.

[ChrisDenton]

My current feeling is that this is unnecessary for Windows specifically. Having a slice of 4,294,967,295 empty buffers followed by one or more non-empty buffers isn't a particularly realistic scenario and is almost certainly a user error. We could document the limit but I'm not so sure it's worth having special handling for it.

[thaliaarchi]

My current feeling is that this is unnecessary for Windows specifically.

That certainly seems reasonable, as it would require 64 GiB to create a slice of that many buffers. I think the only useful case would be if Windows forbids a 0-length slice of buffers. I'll have to look through Windows docs for that.