Take::chunks_vectored breaks the API contract of providing all slices

[vorner]

I'm using bytes 1.0.1. It seems the Take::chunks_vectored method doesn't follow the contract from the trait about providing all the chunks that can fit. From documentation.

If chunk_vectored does not fill every entry in dst, then dst is guaranteed to contain all remaining slices in self.

This code fails, as it fills only a single slice, but there are two in there:

use std::io::IoSlice;

use bytes::Buf;

fn main() {
    let b1: &[u8] = b"Hello ";
    let b2: &[u8] = b"World";
    let c = b1.chain(b2);
    let c = c.take(11);

    assert_eq!(c.remaining(), 11);
    assert_eq!(c.chunk().len(), 6); // There's more than one chunk

    let mut slices = [IoSlice::new(&[]); 2];
    assert_eq!(c.chunks_vectored(&mut slices), 2);
}

(I think this is because it uses the default implementation that just submits one chunk).

I'd submit a fix, but I'm not 100% sure what the right way to do it is. Shall one go by calling the inner chunks_vectored and then postprocess by truncating the right slice and limiting the returned number? Is it OK to overwrite the further slices the caller provided?

[Darksonn]

Tricky. Yeah we probably need to call chunks_vectored on the inner type, then truncate the appropriate slice if too long.

[vorner]

OK, I'll give it a shot.

[seanmonstar]

If chunk_vectored does not fill every entry in dst, then dst is guaranteed to contain all remaining slices in self.

I think this guarantee is wrong. The method chunks_vectored has a default implementation, there's no way we can actually promise the default does this.

[vorner]

Well, one could argue that the default implementation is mere convenience for the cases where it is correct, because it's likely most of the time, but the implementer is still responsible for making sure it holds for their implementation (that is, make sure to specialize in case it is incorrect). Furthermore, I'm not sure removing the guarantee is not a breaking change, strictly speaking.

Anyway, I guess it's still worth fixing for Take either way, even if it is decided to relax the guarantee.

[seanmonstar]

one could argue that the default implementation is mere convenience for the cases where it is correct [..] but the implementer is still responsible for making sure it holds for their implementation

I wouldn't personally agree with that argument. It's far too easy to make a wrapper, impl Buf, and not remember to forward the method. That shouldn't mean you've broken some guarantee.

I'm not sure removing the guarantee is not a breaking change, strictly speaking.

If the guarantee is wrong, I'd see it as just a "docs bug fix".

[vorner]

I totally agree it's easy to miss it, that it's a bit of a trap for the implementer. It happened to me 😇 and that's why I went around looking into the types in here, out of curiosity how these are handled… and turned out it isn't.

On the other hand, that guarantee kind of makes sense from the caller point of view. And provided it was in the documentation, any code relying on that for correctness (for example, stopping iteration once it doesn't fill all the IoSlices) would be correct according to the current state (with the bug in the implementation of Buf), while after removing it would be the caller code that would be broken. Which, technically, breaks previously „correct“ code, which sounds a bit like a breaking change.

What I'm trying to say is not that I'm against removing the guarantee, but it doesn't feel as clear-cut. It feels like both keeping it and removing it has its own set of downsides.

Anyway, that certainly is not my call to make 😇.

[carllerche]

I think the docs were a mistake. That comment was very old. The guarantee obviously never held (as reported here) nor can we make it hold. This is a case of misdocumentation. We should remove that bit from the doc.

[jaskij]

Take here uses the default trait impl from Buf, and it's there where the issue is.

One of the use cases here is peeking into non contiguous Bufs. Which, currently, this is the only method to do it. And since Buf does not provide a guarantee of being contiguous, a way of peaking beyond the first chunk would be useful. It could be some sort of advanced data structures, or simply a linked list of Bytes.

The problem here is that it's impossible to provide a default implementation here which works for all implementations I see a few options here:

• remove or weaken the guarantee (doesn't break API, but what is the use case for this?)
• drop chunks_vectored() entirely (breaks API)
• remove the default implementation from Buf (breaks API)
• provide a default implementation, but require the impl to provide a method to iterate over chunks (breaks API)
• provide a default implementation with extra assertions (only breaks code for broken uses - which is good)

Except the last, all the options are self-explanatory. And break the API. Adding extra assertions add overhead and would probably miss some cases (especially if impl isn't well-tested). This of course requires more thought, but one possible option is below.

fn chunks_vectored<'a>(&'a self, dst: &mut [IoSlice<'a>]) -> usize {
    if dst.is_empty() {
        return 0;
    }

    if self.has_remaining() {
        let c = self.chunk();
        assert_eq!(chunk.len(), self.remaining(),
            "first chunk does not include all contents, please reimplement chunks_vectored()");
        dst[0] = IoSlice::new(c);
        1
    } else {
        0
    }
}