io: fix 'Seek::poll' when the underlying 'AsyncRead::start_seek' call immediately returns Ready.

[jebrosen]

This is the case with Cursor, for example.

EDIT: This is also the case with File if it is not already busy.

[jebrosen]

I'm not familiar with what should happen in the mocked file tests - any pointers?

[jebrosen]

After some debugging, I think I figured out what's going on.

#[test]
fn write_seek_write_err() {
    let (mock, file) = sys::File::mock();
    mock.write_err().seek_start_ok(0);

    let mut file = File::from_std(file);

    let mut t = task::spawn(file.write(HELLO));
    assert_ready_ok!(t.poll());

    pool::run_one();
    // file is now "Busy" with the write completed (with error)

    {
        // NB: the underlying `AsyncSeek::start_seek` implementation for `File` retrieves (but does not return) that write error and then return `Ready(Ok(()))`
        let mut t = task::spawn(file.seek(SeekFrom::Start(0)));

        // BEFORE this PR: The `Seek` future gets `Ready(Ok(()))` and returns Pending, even though nothing will ever wake the task. That's the bug I'm trying to fix.
        // AFTER this PR: The `Seek` future gets `Ready(Ok(()))`, then tries `poll_complete` immediately which returns the write error. So this assert fails.
        assert_pending!(t.poll());
    }

    pool::run_one();

    let mut t = task::spawn(file.write(FOO));
    // BEFORE: t.poll() now (finally) returns the error from the first attempted write
    assert_ready_err!(t.poll());
}

I am not sure what the right way to fix this should be. I can change what the test tests, but I am uncertain on when write errors should be returned immediately and when they should be stashed for a later poll_* call to return.

[jebrosen]

After looking over the other code, I've changed AsyncSeek for File such that:

• write errors are simply "stashed" by all poll_ functions, except for a poll_write
• write errors are only ever returned by poll_write or poll_flush

If we want poll_seek or poll_complete to return pending write errors, I can pursue that approach instead.

[hawkw]

I think saving write errors to return from the AsyncWrite implementation rather than from AsyncSeek is the right thing to do, especially given that the AsyncRead implementation for File already does this:

tokio/tokio/src/fs/file.rs

Lines 69 to 71 in 64dba7a

	/// Errors from writes/flushes are returned in write/flush calls. If a write
	/// error is observed while performing a read, it is saved until the next
	/// write / flush call.

tokio/tokio/src/fs/file.rs

Lines 477 to 481 in 64dba7a

	Operation::Write(Err(e)) => {
	assert!(self.last_write_err.is_none());
	self.last_write_err = Some(e.kind());
	self.state = Idle(Some(buf));
	}

I notice that AsyncRead panics if another write error occurs while self.last_write_error is Some, but AsyncSeek's methods do not:

tokio/tokio/src/fs/file.rs

Lines 529 to 531 in 64dba7a

	Operation::Write(Err(e)) => {
	self.last_write_err = Some(e.kind());
	}

and

tokio/tokio/src/fs/file.rs

Lines 550 to 552 in 64dba7a

	Operation::Write(Err(e)) => {
	self.last_write_err = Some(e.kind());
	}

Do you think we should change that in this PR as well?

[hawkw]

nit, take it or leave it: these lines could be collapsed into just

poll => poll,

or similar. Not a blocker... perhaps the current code is clearer.

[jebrosen]

This one actually can't be, because the left poll is Result<(), _> and the right side poll must be Result<u64, _>. Something like poll => poll.map(|r| Err(r.unwrap_err())), works but is more confusing IMO

[jebrosen]

I notice that AsyncRead panics if another write error occurs while self.last_write_error is Some

IIUC, that panic should actually be unreachable because last_write_error of Some should have been returned instead of transitioning to Busy(Operation::Write). But I'm not entirely confident that's the case. Overall, I did wish the state transitions were a bit more consolidated (i.e. a hypothetical poll_complete_inflight) while trying to fix this. For now, I'll copy that assert over for consistency.

[notriddle]

write errors are simply "stashed" by all poll_ functions, except for a poll_write
write errors are only ever returned by poll_write or poll_flush

Yeah, your version probably makes more sense than mine did. Thanks for fixing my broken code!

[jebrosen]

Rebased so the two changes are clear without the noise commits. I think all this is missing is additional tests if desired.

I also don't know who should press which kind of merge button once this is ready.

[notriddle]

I have a suggestion. Let's add a test case.

use std::io::Cursor;
use tokio::prelude::*;

#[tokio::main]
async fn main() -> io::Result<()> {
    let mut cursor = Cursor::new(b"abcdefg".iter().copied().collect::<Vec<_>>());

    // the `seek` method is defined by this trait
    cursor.seek(SeekFrom::Start(3)).await?;

    let mut buffer = [0; 1];
    let n = f.read(&mut buffer[..]).await?;
    assert_eq!(buf, [b'd']);

    Ok(())
}

[jebrosen]

Added a test/example - I think I am finally happy with this PR.