write_all doesn't guarantee data to be written after returning Poll::Ready

[mineichen]

Version
Affects all tokio-versions since 1.0

Platform
Veryfied on

• 5.15.81-1-MANJARO x86_64 GNU/Linux (1.65)
• 64-bit Windows 11 (RUST 1.67.1)

Description
tokio::io::AsyncWriteExt::write_all doesn't guarantee to write all data on it's completion. The following sometimes fails in the first cycle, sometimes after several hundred iterations:

[dependencies]
futures = "0.3.25"
tokio = {version = "1.26.0", features = ["fs", "io-util", "macros", "rt", "time"]}
tokio-util = {version = "0.7.7", features = ["compat"]}

#[cfg(test)]
mod tests {

    #[tokio::test]
    async fn tokio_read_write() -> Result<(), Box<dyn std::error::Error>> {
        for i in 0..10_000 {
            let filepath = "settings.json";
            tokio::fs::File::create(filepath).await?;
            {
                // By using this instead of tokio::write_all, the test alwas exits successfully 
                // std::io::Write::write_all(&mut std::fs::File::create(filepath)?, b"test")?;
                
                tokio::io::AsyncWriteExt::write_all(
                    &mut tokio::fs::File::create(&filepath).await?,
                    b"test",
                )
                .await?; 

                // Is broken too:
                /*
                futures::AsyncWriteExt::write_all(
                    &mut tokio_util::compat::TokioAsyncWriteCompatExt::compat_write(
                        tokio::fs::File::create(&filepath).await?,
                    ),
                    b"test",
                )
                .await?; */
            }
            let mut data = Vec::new();
            // Both commands fail with tokio::write_all
            // let count = std::fs::File::open(filepath).and_then(|mut f| f.read_to_end(&mut data));
            let count = match tokio::fs::File::open(filepath).await {
                Ok(mut f) => tokio::io::AsyncReadExt::read_to_end(&mut f, &mut data).await,
                Err(e) => Err(e),
            };

            if let Ok(x) = count {
                if x == 0 {
                    let now_it_might_work = match tokio::fs::File::open(filepath).await {
                        Ok(mut f) => tokio::io::AsyncReadExt::read_to_end(&mut f, &mut data).await,
                        Err(e) => Err(e),
                    };
                    panic!("READ bytes run {i}: {:?}", now_it_might_work);
                }
            }
        }
        Ok(())
    }
}

Ideas
The implementation WriteAll seems suspicious to me:

impl<W> Future for WriteAll<'_, W>
where
    W: AsyncWrite + Unpin + ?Sized,
{
    type Output = io::Result<()>;

    fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<io::Result<()>> {
        let me = self.project();
        while !me.buf.is_empty() {
            let n = ready!(Pin::new(&mut *me.writer).poll_write(cx, me.buf))?;
            {
                let (_, rest) = mem::take(&mut *me.buf).split_at(n);
                *me.buf = rest;
            }
            if n == 0 {
                return Poll::Ready(Err(io::ErrorKind::WriteZero.into()));
            }
        }

        Poll::Ready(Ok(()))
    }
}

Shouldn't the last line be replaced with the following?

Pin::new(&mut *me.writer).poll_flush(cx)

If so, futures::AsyncWriteExt::write_all has to be patched as well.

Severeness
I think that this behavior is very unexpected and thus dangerous. If it cannot be fixed, it should at least be documented in the docs.

EDIT 30min later:
A call to tokio::io::AsyncWriteExt::flush(&mut file).await?; after the write_all solves the issue. I see why the flush doesn't happen automatically. This is redundant work on consecutive calls to write_all. I would however not expect a helper-function to require me to call flush afterwards. Imho there should be a write_all_unflashed with the current implementation and the write_all should contain the flush. Code could still be optimized without users running into this trap.

[Darksonn]

It is correct that Tokio files need to be flushed.

Having write_all emit a flush is not an option. For one, it is designed to mirror the standard library. For another, it would make it difficult to use a buffered writer correctly, whose performance relies on the fact that you don't flush after every write. For a third, it would be a breaking change.

We have made changes to improve the situation when it comes to runtime shutdown (see #4316), but nothing more can be done here.

[mineichen]

1. Unfortunately, the standard-library doesn't seem to require flush while tokio does. So it is not consistent.
2. BufferedWriter could still use the write_all_unflushed internally, so it's no problem to implement buffered_writer.
3. I aggree that it's a breaking change, but not on the API-surface. You could argue that every bugfix is a breaking change if someone relies on it. I don't assume, that anybody relies on the buffer not being flushed.

Why do we even have the concept of unflashed writes? Could this usecases not be covered by the "write_vectored"-API? I know that this is out of scope for this project but I think that this question should still be raised. To maintain the API, flush could just become a noop...

edit
Maybe it's not the WriteExt which should be changed, but the File itself to be closer to the std-implementation

[Darksonn]

While its true that it is file that differs, the fact that the OS provides no async file API means that it cannot be fixed.