Tracking issue for io_slice_advance

[Thomasdezeeuw]

This is a tracking issue for IoSlice::{advance, advance_slices} and IoSliceMut::{advance, advance_slices}.

Feature gate: #![feature(io_slice_advance)].

Steps:

• Implement the RFC.
  • Add {IoSlice, IoSliceMut}::advance #62987
  • Rename IoSlice(Mut)::advance to advance_slice and add IoSlice(Mut)::advance #85802
• FCP
• Stabilization PR.

Current API additions:

impl<'a> IoSlice<'a> { // And `IoSliceMut`
    pub fn advance(&mut self, n: usize);
    pub fn advance_slices(bufs: &mut &mut [IoSlice<'a>], n: usize);
}

---

Old issue:

Writing rust-lang/futures-rs#1741 I needed to resort to unsafe code to change the underlying slice in IoSlice (and IoSliceMut). I'm missing a method that can change the underlying slice. @Nemo157 said that I should open an issue.

Current idea would be something like Buf::advance from the bytes crate.

impl IoSlice {
    // Advance the internal cursors of the slice by `n` bytes.
    fn advance(&mut self, n: usize) {
        // ..
    }
}

[Thomasdezeeuw]

Looking for some feedback as to whether this would be accepted.

[Thomasdezeeuw]

Can someone reopen this as the tracking issue for io_slice_advance? I've updated the the first comment, but I'm not sure it's 100% correct.

[seanmonstar]

I was trying to impl Buf for IoSlice in the bytes crate, and noticed there's currently an issue with lifetimes:

impl Buf for IoSlice<'_> {
    fn advance(&mut self, cnt: usize) {
        *self = IoSlice::new(&self[cnt..]); // conflicting lifetime requirements
    }
}

To allow such a thing externally, we could add a method to IoSlice that gives the slice with the same lifetime as IoSlice (whereas Deref uses an anonymous lifetime).

impl<'a> IoSlice<'a> {
    pub fn inner_ref(&self) -> &'a [u8] {
        // ...
    }
}

[Thomasdezeeuw]

@seanmonstar can get around the lifetime issues by replacing self with an empty slice and then replacing it again with the advanced slice? Something like can be found here: https://github.com/rust-lang/rust/pull/62987/files#diff-668f8f358d4a93474b396dcb3727399eR975.

[seanmonstar]

@Thomasdezeeuw I don't think so. It's not possible to get the inner slice with the same lifetime due to Deref:

let tmp = std::mem::replace(self, IoSlice::new(&[]));
let tail = &tmp[cnt..]; // <- lifetime isn't same as `IoSlice<'a>`
*self = IoSlice::new(tail);

[Nemo157]

@seanmonstar it's not an amazing API to build that on top of, but it is possible (playground):

let mut slices = [std::mem::replace(self, IoSlice::new(&[]))];
let slices = IoSlice::advance(&mut slices[..], cnt);
match slices {
    [slice] => {
        *self = std::mem::replace(slice, IoSlice::new(&[]));
    }
    [] => {
        *self = IoSlice::new(&[]);
    }
    _ => unreachable!(),
}

[seanmonstar]

Oh right, that is using the unstable function. I was referring to be able to implement this now.

[Nemo157]

Ah, I just assumed since that's what the tracking issue is about. It seems like having IoSlice<'a>::into_inner(self) -> &'a [u8] along with Clone and Copy would make sense since it's just a newtyped slice (hopefully that's possible with the actual os-specific implementations).

[mpdn]

I've been trying to write a write_all_vectored function that continuously calls write_vectored, but it seems like this is not really possible to do without some way of advancing the IoSlices (or resorting to unsafe, of course). It does not seem to me like vectored IO is very practical without this feature or something equivalent.

[Ekleog]

Are there any news on landing this? I'm trying to do vectored IO too, and it would be very nice if it were possible to just use this function — I think this is the only unstable feature I currently depend on, and it would be very nice to be able to compile on stable without having to add unsafe code.

into_inner as discussed above is another primitive that would make it possible to implement advance as a user, and it would most likely be great to have it, but seeing the way vectored IO is done I can't see a world where a function like advance isn't required.

[kalcutter]

Are there any concerns with the current panic behavior? If not, can this be FCPed?

[PureWhiteWu]

Really looking forward for this being stabilized. This is quite useful in network programming.

[dead-claudia]

I do have one quick question prior to it stabilizing: do you want to require people to do if bufs.is_empty()/if buf.is_empty(), or should both methods return booleans, returning true if the buffer is non-empty and false otherwise? The latter would be make calling code a little easier to follow IMHO, since you can just use it as the while condition in most loops:

fn write_all_vectored(&mut self, mut bufs: &mut [IoSlice<'_>]) -> Result<()> {
    // Advance initially with `n = 0` to guarantee that bufs is empty if it
    // contains no data, to avoid calling write_vectored if there is no data
    // to be written.
    let mut n = 0;
    while IoSlice::advance_slices(&mut bufs, n) {
        match self.write_vectored(bufs) {
            Ok(0) => {
                return Err(error::const_io_error!(
                    ErrorKind::WriteZero,
                    "failed to write whole buffer",
                ));
            }
            Ok(bytes) => n = bytes,
            Err(ref e) if e.kind() == ErrorKind::Interrupted => {}
            Err(e) => return Err(e),
        }
    }
    Ok(())
}

The code itself already necessarily checks for this for the outer buffer, and it's pretty trivial for the inner buffer.

[kalcutter]

How can progress be made on this issue? It is unclear to me what concerns are blocking this.

[Thomasdezeeuw]

@kalcutter it hasn't seen any changes for a year and half (?), so I think someone just needs to create a stabilisation pr.

[eduardosm]

I think a FCP is needed first.

[Thomasdezeeuw]

I think a FCP is needed first.

Isn't that usually done in the stabilization pr?

[eduardosm]

I'm not sure, but I have usually seen FCPs in tracking issues.

[andll]

+1 for starting FCP, this is a really useful feature

[Amanieu]

This is a generally useful API for working with vectored I/O.

@rfcbot fcp merge

---

I do have one quick question prior to it stabilizing: do you want to require people to do if bufs.is_empty()/if buf.is_empty(), or should both methods return booleans, returning true if the buffer is non-empty and false otherwise? The latter would be make calling code a little easier to follow IMHO, since you can just use it as the while condition in most loops:

I think making advance/advance_slices return a bool would be confusing, it isn't immediately clear from the function name what the return value means. Code is much clearer if it explicitly queries is_empty on the slice instead.

[rfcbot]

Team member @Amanieu has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

[BurntSushi]

The advance method is an inherent method on a type that implements Deref for [u8]. Don't we usually try to avoid adding new inherent methods in that case? (I suppose this would only become an issue if we added a method named advance to [u8]. Seems unlikely but not completely impossible?)

[Mark-Simulacrum]

FWIW, bytes provides advance on &[u8] via https://docs.rs/bytes/latest/bytes/trait.Buf.html. Is there a reason not to put advance directly on &[u8] here? I think that would be slightly incompatible to move later...

[dead-claudia]

FWIW, bytes provides advance on &[u8] via https://docs.rs/bytes/latest/bytes/trait.Buf.html. Is there a reason not to put advance directly on &[u8] here? I think that would be slightly incompatible to move later...

Arguably it's inconsistent, since the other method takes a slice of IoSlices.

However, I could see some benefit of moving both to methods via a pair of extension traits (say, ReadTarget and WriteTarget):

• You could provide overloads for both IoSlice/IoSliceMut and &[u8]/&mut [u8], all using the same name.
• These traits could also include ReadTarget::read_from(&mut self, impl Read) and WriteTarget::write_into(&mut self, impl Write) methods, and those could be used to be polymorphic over normal and vectorized reads, easing migration between the two a lot.

[Thomasdezeeuw]

FWIW, bytes provides advance on &[u8] via https://docs.rs/bytes/latest/bytes/trait.Buf.html. Is there a reason not to put advance directly on &[u8] here? I think that would be slightly incompatible to move later...

Arguably it's inconsistent, since the other method takes a slice of IoSlices.

That's because the advance_slices method works on a slice of IoSlice (hence the name), thus needs mutable access to &mut [IoSlice], which means &mut &mut [IoSlice], which doesn't work with a variant of &mut self.

That said, I'm perfectly fine with changing fn advance(&mut self, n: usize) to fn advance(this: &mut IoSlice<'a>, n: usize) to avoid this issue mentioned by @BurntSushi.

[wx-csy]

rust/library/std/src/io/mod.rs

Lines 1331 to 1354 in 55e5c9d

	#[unstable(feature = "io_slice_advance", issue = "62726")]
	#[inline]
	pub fn advance_slices(bufs: &mut &mut [IoSlice<'a>], n: usize) {
	// Number of buffers to remove.
	let mut remove = 0;
	// Total length of all the to be removed buffers.
	let mut accumulated_len = 0;
	for buf in bufs.iter() {
	if accumulated_len + buf.len() > n {
	break;
	} else {
	accumulated_len += buf.len();
	remove += 1;
	}
	}
	*bufs = &mut take(bufs)[remove..];
	if bufs.is_empty() {
	assert!(n == accumulated_len, "advancing io slices beyond their length");
	} else {
	bufs[0].advance(n - accumulated_len)
	}
	}
	}

I think we may use checked_add to compute accumulated_len. Because the contents of IoSlice may alias, it is theoretically possible that the total lengths of the IoSlices exceed usize::MAX, causing an overflow on accumulated_len.

[eduardosm]

I opened #116070 to handle that.

[thomaseizinger]

FWIW, bytes provides advance on &[u8] via docs.rs/bytes/latest/bytes/trait.Buf.html. Is there a reason not to put advance directly on &[u8] here? I think that would be slightly incompatible to move later...

Arguably it's inconsistent, since the other method takes a slice of IoSlices.

That's because the advance_slices method works on a slice of IoSlice (hence the name), thus needs mutable access to &mut [IoSlice], which means &mut &mut [IoSlice], which doesn't work with a variant of &mut self.

That said, I'm perfectly fine with changing fn advance(&mut self, n: usize) to fn advance(this: &mut IoSlice<'a>, n: usize) to avoid this issue mentioned by @BurntSushi.

Although it is also still unstable, a related set of functions on MaybeUninit uses a different naming pattern: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.slice_as_bytes

impl<T> MaybeUninit<T> {
    pub fn slice_as_bytes(this: &[MaybeUninit<T>]) -> &[MaybeUninit<u8>];
}

Applying the same here would read kind of odd though because of the repetition of slice:

impl<T> IoSlice<T> {
	pub fn slice_advance(this &mut &mut [IoSlice<'a>], n: usize);
}

// in some impl
IoSlice::slice_advance(&mut bufs, n_read);

Perhaps we could name them advance_one and advance_many?

[Thomasdezeeuw]

Although it is also still unstable, a related set of functions on MaybeUninit uses a different naming pattern: https://doc.rust-lang.org/std/mem/union.MaybeUninit.html#method.slice_as_bytes

impl<T> MaybeUninit<T> {
    pub fn slice_as_bytes(this: &[MaybeUninit<T>]) -> &[MaybeUninit<u8>];
}

I fail to see the relation between the two functions. MaybeUninit:: slice_as_bytes does a (type) conversion, while IoSlice::slice_advance manipulates the slices, I don't think they should follow the same naming pattern.

Can you elaborate why you think they should?

[thomaseizinger]

I fail to see the relation between the two functions. MaybeUninit:: slice_as_bytes does a (type) conversion, while IoSlice::slice_advance manipulates the slices, I don't think they should follow the same naming pattern.

Can you elaborate why you think they should?

as_bytes is the "action" of the function and equivalent of "advance" for IoSlice. It is prefixed by slice_ because the first argument isn't self but a slice of self. Following that logic, the functions should be:

impl IoSlice {
	fn advance(&mut self, n: usize);
	fn slice_advance(this &mut &mut [IoSlice<'a>], n: usize);
}

Perhaps it should be a free-function instead and we offer only IoSlice::advance?

[thomaseizinger]

It is also worth noting that AsyncWrite::poll_write_vectored and Write::write_vectored only give you a &[IoSlice<'_>]. How are implementations meant to advance these anyway?

[Thomasdezeeuw]

I fail to see the relation between the two functions. MaybeUninit:: slice_as_bytes does a (type) conversion, while IoSlice::slice_advance manipulates the slices, I don't think they should follow the same naming pattern.
Can you elaborate why you think they should?

as_bytes is the "action" of the function and equivalent of "advance" for IoSlice. It is prefixed by slice_ because the first argument isn't self but a slice of self. Following that logic, the functions should be:

impl IoSlice {
	fn advance(&mut self, n: usize);
	fn slice_advance(this &mut &mut [IoSlice<'a>], n: usize);
}

I still fail to see the relation, but those names seem fine with me as well. At this point I don't really care about the names to be honest.

It is also worth noting that AsyncWrite::poll_write_vectored and Write::write_vectored only give you a &[IoSlice<'_>]. How are implementations meant to advance these anyway?

See io::Write::vectored_all, write_vectored shouldn't advance the buffers.