BufReader<impl AsyncWrite + AsyncSeek> panics when calling write_all and immediately seek

[hack3ric]

Version
tokio v1.19.2 and v1.20.1
tokio-util v0.7.3

Platform
macOS 12.3 x86_64

Description
The following code panics:

use std::io::SeekFrom;
use tokio::fs::File;
use tokio::io::{self, AsyncSeekExt, AsyncWriteExt, BufReader};

#[tokio::main]
async fn main() -> io::Result<()> {
    let mut x = BufReader::new(File::create("test").await?);
    x.write_all(b"test").await?;
    x.seek(SeekFrom::Start(0)).await?;
    Ok(())
}

thread 'main' panicked at 'must wait for poll_complete before calling start_seek', /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/fs/file.rs:570:28
stack backtrace:
   0: std::panicking::begin_panic
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/panicking.rs:616:12
   1: <tokio::fs::file::File as tokio::io::async_seek::AsyncSeek>::start_seek
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/fs/file.rs:570:28
   2: <tokio::io::util::buf_reader::BufReader<R> as tokio::io::async_seek::AsyncSeek>::poll_complete
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/io/util/buf_reader.rs:242:17
   3: <&mut T as tokio::io::async_seek::AsyncSeek>::poll_complete
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/io/async_seek.rs:56:13
   4: <tokio::io::seek::Seek<S> as core::future::future::Future>::poll
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/io/seek.rs:49:25
   5: test_mlua::main::{{closure}}
             at ./src/main.rs:9:31
   6: <core::future::from_generator::GenFuture<T> as core::future::future::Future>::poll
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/future/mod.rs:91:19
   7: tokio::park::thread::CachedParkThread::block_on::{{closure}}
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/park/thread.rs:263:54
   8: tokio::coop::with_budget::{{closure}}
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/coop.rs:102:9
   9: std::thread::local::LocalKey<T>::try_with
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:445:16
  10: std::thread::local::LocalKey<T>::with
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/std/src/thread/local.rs:421:9
  11: tokio::coop::with_budget
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/coop.rs:95:5
  12: tokio::coop::budget
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/coop.rs:72:5
  13: tokio::park::thread::CachedParkThread::block_on
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/park/thread.rs:263:31
  14: tokio::runtime::enter::Enter::block_on
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/runtime/enter.rs:152:13
  15: tokio::runtime::thread_pool::ThreadPool::block_on
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/runtime/thread_pool/mod.rs:90:9
  16: tokio::runtime::Runtime::block_on
             at /Users/hacker/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/tokio-1.20.0/src/runtime/mod.rs:484:43
  17: test_tokio::main
             at ./src/main.rs:10:5
  18: core::ops::function::FnOnce::call_once
             at /rustc/e092d0b6b43f2de967af0887873151bb1c0b18d3/library/core/src/ops/function.rs:248:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[Darksonn]

One way to fix this is to have BufReader flush the IO resource before attempting to seek.

[Darksonn]

The documentation for AsyncSeek specifies that an error should be returned if the seek cannot be started. It is incorrect for File to panic in this situation.

[sebpuetz]

I was looking into this issue and opened a PR at #4897 to change the panic to an error.

Although the File impl seems incorrect, the behavior between BufReader and BufWriter's poll_complete impl is not consistent, i.e. BufWriter flushes its buffer before seeking:

tokio/tokio/src/io/util/buf_writer.rs

Lines 244 to 245 in 9d9488d

	// Flush the internal buffer before seeking.
	ready!(self.as_mut().flush_buf(cx))?;

Do you think it makes sense to also change BufReader to flush the underlying writer before attempting to seek? It would probably make the error in the snippet provided by @hack3ric less surprising

[Darksonn]

Actually, I realize now that BufReader will not be able to flush before seeking. We don't require AsyncWrite for AsyncSeek to be implemented on BufReader. If we make any further changes, it should probably be making File::start_seek work even if there is an active operation (other than a seek, that is).

[sebpuetz]

Looking into File::start_seek I don't see an obvious change that would make the function work on a File busy with read / write. Conceptually, the seek operation needs to be queued but File's state only has a single slot that's already occupied by the pending operation.

Even if the JoinHandle in State::Busy was tagged with the type of pending operation (i.e. make it available outside the running blocking task), we wouldn't be able to start the seek and appropriately communicate that to the caller - we'd still have to return an error that we were unable to submit the operation

[Darksonn]

If we were to do that, then we would need to add an additional field to Busy to track that a seek should be started whenever the currently busy operation finishes. The poll_complete method (and probably poll_flush too) should then wait for the busy operation to complete, then submit the seek operation, then wait for the seek operation to complete.

[jacobsimpson]

It looks like the PR above (#4897) has been merged. When I run the sample code from the initial post against version 1.21.0, it now comes back as an error result from the seek line, instead of a panic. Is this issue considered fixed?

[Darksonn]

Yes, thanks for pointing that out.