Implement is_empty() for BufReader

[fintelia]

Simple implementation of is_empty method for BufReader so it is possible to tell whether there is any data in its buffer.

I didn't know correct stability annotation to place on the function. Presumably there is no reason to place this feature behind a feature flag, but I wasn't sure how to tag it as an unstable feature without that.

CC: #45323

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[sfackler]

What would code do in response to the emptyness or non-emptyness of the buffer?

[fintelia]

If the buffer is non-empty, then the application might want to read data from it. However, if the buffer is empty the application might not want to issue a potentially expensive (or blocking) call to the underlying Reader just yet. It could also be issued before into_inner to ensure that no data will be lost.

In my case, I'm using some BufReaders to wrap non-blocking TcpStreams which I poll over using mio. Whenever a message arrives on one of them I read it from the indicated stream. However, to avoid the possible deadlock of polling on the underlying stream for an already buffered message, I must empty the buffer before polling again. But I can't know the buffer is empty without calling read() and having it return zero bytes, which means that I end up doing a bunch of extraneous syscalls just to learn the state of a userspace buffer.

[carols10cents]

r? @sfackler (i think alex was traveling and is traveling)

[fintelia]

It is also worth considering if it would make sense to have a len or get_buffer method instead or in addition. Any of these could be implemented in terms of the others using the existing fill_buffer method (which fills the buffer iff it is currently empty, and returns a reference to it either way).

[carols10cents]

ping @rust-lang/libs, could one of yinz be a new reviewer on this please?

[BurntSushi]

In my case, I'm using some BufReaders to wrap non-blocking TcpStreams which I poll over using mio. Whenever a message arrives on one of them I read it from the indicated stream. However, to avoid the possible deadlock of polling on the underlying stream for an already buffered message, I must empty the buffer before polling again. But I can't know the buffer is empty without calling read() and having it return zero bytes, which means that I end up doing a bunch of extraneous syscalls just to learn the state of a userspace buffer.

Could you expand on this a bit? I don't think I understand it. For example, I don't know why deadlock is a concern here.

[fintelia]

@BurntSushi Consider the following sequence of events:

1. Client A sends two messages, and waits for replies to both
2. The server wakes up when epoll detects that there is data to be read off client A's TCP stream
3. The server calls BufReader::read() and gets back the first message. However, the BufReader has actually read both messages into its internal buffer using a single read syscall.
4. The server replies to the first message.
5. Then the server calls epoll again. Since there is no data in the kernel waiting to be read, it blocks forever.
6. Client A gets the first reply, but still block waiting for the second.

At this point neither the server nor the client can make forward progress, and are thus deadlocked. In fact, even if the server gets messages from other clients it still won't become unblocked, because it has no reason to check for messages from client A.

[carols10cents]

r? @BurntSushi, does @fintelia's comment explain things sufficiently?

[BurntSushi]

@fintelia Thanks for clarifying! I think that motivation seems good enough for me. @bors r+

[bors]

📌 Commit df4b781 has been approved by BurntSushi

[BurntSushi]

@carols10cents Thanks for the ping. :-)

[bors]

⌛ Testing commit df4b781 with merge 58557fa...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: BurntSushi
Pushing 58557fa to master...

[jonhoo]

@BurntSushi this has feature = "bufreader_is_empty". Should an entry be added to the unstable book? Or the feature gate be removed?