Discrepancy between docs and implementation of BufReader::fill_buf

[jonhoo]

Would you like to fix this bug? If so, here's how.

---

The documentation currently states that BufRead::fill_buf:

Fills the internal buffer of this object, returning the buffer contents.

However, this is not what BufReader's implementation of that method does. It will only fill the buffer if no data is currently buffered. If data is already present in the buffer, no filling takes place:

impl<R: Read> BufRead for BufReader<R> {
    fn fill_buf(&mut self) -> io::Result<&[u8]> {
        if self.pos >= self.cap {
            self.cap = self.inner.read(&mut self.buf)?;
            self.pos = 0;
        }
        Ok(&self.buf[self.pos..self.cap])
    }
}

I would argue that the current behavior of BufReader's implementation is usually what users would want to do, but if does not match the documentation of, nor the name of, fill_buf. I don't know what the right solution here is, but I suspect the only safe thing to do would be to update BufReader's implementation to match the docs (i.e., to make it actually fill its buffer), and then to add another method to BufRead (perhaps buf_bytes?) that gives the currently buffered contents, regardless of the amount of buffered data, and without reading from the underlying Read.

[abonander]

Adding a method to BufRead would be a breaking change unless you have a suitable default implementation in mind.

I would love it if we can make "every time this is called, the implementation should fetch more data" part of the fill_buf() API contract because it's easy to get burned when you make that assumption with BufReader. The only one impl I know of where this guarantee isn't really feasible is io::Chain which can only return buffers from one source at a time.

[jonhoo]

Well, even that is tricky, right? What if the buffer is already full? I also think there's value in being able to get access to the bytes currently in the buffer without doing a syscall (something like get_buf).

[abonander]

The issue is that you can't really do anything to the structure of the BufRead trait at this point without breaking changes, though this is now feasible with epochs. Adding extra functionality directly to BufReader is quite a bit more reasonable though. I have an old RFC that I couldn't be bothered to champion and instead just reimplemented it in my own crate.

[jonhoo]

I guess one question is where we decide to not expand io::BufReader and say that external crates should provide features instead. I'd advocate for this being something io::BufReader should support (though as you say, possibly not io::BufRead).

[twimpiex]

I also ran into this 'faulty' documentation issue.
I have a program which needed pushing back data on the reader and by reading the documentation I thought I could use fill_buf with consume for that, but found out the hard way that fill_buf isn't actually filling the buffer but returning the existing buffer if something is still in there.
Hardly appreciated if the fill_buf acts as documented.

[sfackler]

The only one impl I know of where this guarantee isn't really feasible is io::Chain which can only return buffers from one source at a time.

The proposed behavior is fundamentally broken. Imagine a BufReader<TcpStream> being used to parse some line-oriented protocol. The BufReader's buffer capacity is 4096 bytes, and the peer sends a single TCP packet with 2 lines in 100 bytes. It then expects our end to respond, so it's not doing any more writes.

You call read_line, which calls into fill_buf. Since this is a new BufReader, the buffer is empty so it calls down into the TcpStream and reads all 100 bytes into the buffer. read_line then returns the first line, and there's the one remaining line in the buffer.

Now you call read_line again to read the last line the peer sent over. With the current behavior, the BufReader's buffer is nonempty, so fill_buf immediately returns it. read_line returns the last line our end responds to the peer, and everything is happy. With the proposed behavior, fill_buf calls back down into the TcpStream since its buffer isn't full and the protocol deadlocks.

[frewsxcv]

Just to clarify, is the idea here to update the BufRead docs from:

Fills the internal buffer of this object, returning the buffer contents.

...into something like this?

Fills the internal buffer of this object, returning the buffer contents. If the buffer already has data prior to fill_buf being called, the buffer will not grow and the buffer contents will be returned.

[frewsxcv]

Ah, @sfackler commented while I was writing my comment and answered a couple of the questions I had 👍

[sfackler]

I'd probably phrase it as

returns the contents of the internal buffer, filling it with more data from the inner reader if it is empty

[jonhoo]

@sfackler I agree with you that changing the behavior of fill_buf seems wrong. As pointed out in the original post: "the current behavior of BufReader's implementation is usually what users would want to do, but if does not match the documentation of, nor the name of, fill_buf".

I do think there's an argument for providing a method that gives the buffer contents without ever doing a syscall, but that's probably an argument for another issue.

[steveklabnik]

I'm happy to mentor anyone who wants to fix this issue! Here's how:

The line in question is here:

rust/src/libstd/io/mod.rs

Line 1376 in 52ed3d8

/// Fills the internal buffer of this object, returning the buffer contents.

This should be changed to:

Returns the contents of the internal buffer, filling it with more data from the inner reader if it is empty.

I'm happy to answer any additional questions!

[jonhoo]

Perhaps we should also include a reference to the newly added BufReader::buffer (#49139)? Though I suppose that is (sadly) only available for BufReader and not all BufRead.