make tokio::io::empty cooperative

[BraulioVM]

Fixes: #4291

Motivation

Reads and buffered reads from a tokio::io::empty were always marked
as ready. That makes sense, given that there is nothing to wait for.
However, doing repeated reads on the empty could stall the event
loop and prevent other tasks from making progress.

Solution

This change makes reads on empty objects cooperative. One of every two
reads will return Poll::Pending, which will give the executor a
chance to keep making progress elsewhere.

Testing

I've added four integration tests. The tests wouldn't fail if the fix hadn't been implemented,
they would just hang forever (just like the original issue).

This is my first contribution to the project, so maybe there are higher-level ways of achieving
this behaviour and testing this functionality. Let me know if that's the case!

[Darksonn]

What you implemented here doesn't actually integrate with Tokio's coop system. You can see an example of how to do that here (the poll_proceed and made_progress calls). There's an overview of the coop strategy used by Tokio here.

[Darksonn]

Also, seems like you have a warning in the code:

error: unused import: `empty`
 --> tokio/tests/io_util_empty.rs:2:17
  |
2 | use tokio::io::{empty, AsyncBufReadExt, AsyncReadExt};
  |                 ^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`

error: could not compile `tokio` due to previous error

[Darksonn]

These timeouts are too long when you aren't mocking the time driver. Please try reducing them to 5 ms or so.

[Darksonn]

Honestly, selecting on the async block and tokio::task::yield_now would probably work too.

[Darksonn]

And also, welcome!

[BraulioVM]

Thanks for all the feedback and all the pointers!

The coop system makes a lot of sense. I implemented the changes but I believe they won't fix the original issue! That's because of how timeout and Sleep are implemented.

See the future implementation for Timeout

tokio/tokio/src/time/timeout.rs

Lines 163 to 183 in ee4b2ed

	impl<T> Future for Timeout<T>
	where
	T: Future,
	{
	type Output = Result<T::Output, Elapsed>;
	fn poll(self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> Poll<Self::Output> {
	let me = self.project();
	// First, try polling the future
	if let Poll::Ready(v) = me.value.poll(cx) {
	return Poll::Ready(Ok(v));
	}
	// Now check the timer
	match me.delay.poll(cx) {
	Poll::Ready(()) => Poll::Ready(Err(Elapsed::new())),
	Poll::Pending => Poll::Pending,
	}
	}
	}

and poll_elapsed for Sleep

tokio/tokio/src/time/driver/sleep.rs

Lines 379 to 388 in ee4b2ed

	fn poll_elapsed(self: Pin<&mut Self>, cx: &mut task::Context<'_>) -> Poll<Result<(), Error>> {
	let me = self.project();
	// Keep track of task budget
	let coop = ready!(crate::coop::poll_proceed(cx));
	me.entry.poll_elapsed(cx).map(move |r| {
	coop.made_progress();
	r
	})

empty::read may return Polling when its budget decreases to 0, but then me.delay.poll will also always return Polling in that case, even if the timeout has been hit, because Sleep will also observe that it has no budget. When the executor takes another stab at completing the task, with a fresh new budget, it will go back to empty::read, until the budget is exhausted again.

I've observed this behaviour locally after applying the changes. Does this sound reasonable to you?

As for next steps:

1. I could re-implement the tests to use an async block and tokio::task::yield_now, and we could get that merged, but being aware that this change doesn't fix the original issue.
2. I could try fixing the original issue, and I believe this would involve changing the implementation of the Timeout future. Maybe poll the Sleep with an unconstrained budget? I don't know, I'm sure you can come up with better approaches, and that's why I'd prefer to hear from you before working on any specific fix.

What are your thoughts?

[Darksonn]

Thank you for pointing that out, that explains why some other case I was thinking of also doesn't work. We do want to make tokio::io::empty use coop, but we should also fix the timer.

Can you update this PR with the changes I requested for tokio::io::empty, and then open a new issue or PR for the issue with timeout? It seems like we can fix the timeout by having it check the budget before and after polling the task, and having it unconditionally check the timer if the budget was exhausted somewhere inside the task, but still abort if the budget was exhausted before the timeout got polled.

If you want to discuss the changes to timeout further before opening a PR, then please open an issue so we can do it there instead of here.

[BraulioVM]

Sounds like a plan!

[BraulioVM]

I will fix the failing build later today

[BraulioVM]

Is there anything I should do before this gets merged?

[Darksonn]

It looks good.