Builder::finish() does not 'close' inner Writer

[danieleades]

Builder::finish() doesn't seem to flush the state of the inner writer. this means the following snippet doesn't work-

use async_compression::futures::write::GzipEncoder;
use async_std::{fs::File, path::Path};
use async_tar::Builder;
use futures::io::AsyncWrite;

async fn write_archive<W>(writer: W, src_directory: impl AsRef<Path>) -> std::io::Result<()>
where
    W: AsyncWrite + Unpin + Send + Sync,
{
    let mut archive_builder = Builder::new(writer);
    archive_builder.append_dir_all("", src_directory).await?;
    archive_builder.finish().await
}

#[async_std::main]
async fn main() {
    let file = File::create("foo.tar.gz").await.unwrap();
    let encoder = GzipEncoder::new(file);
    write_archive(encoder, "sample-directory").await.unwrap();
}

but this snippet does work

use async_compression::futures::write::GzipEncoder;
use async_std::{fs::File, path::Path};
use async_tar::Builder;
use futures::io::{AsyncWrite, AsyncWriteExt};

async fn write_archive<W>(writer: W, src_directory: impl AsRef<Path>) -> std::io::Result<()>
where
    W: AsyncWrite + Unpin + Send + Sync,
{
    let mut archive_builder = Builder::new(writer);
    archive_builder.append_dir_all("", src_directory).await?;
    archive_builder.finish().await?;

    archive_builder.into_inner().await?.close().await
}

#[async_std::main]
async fn main() {
    let file = File::create("foo.tar.gz").await.unwrap();
    let encoder = GzipEncoder::new(file);
    write_archive(encoder, "docker-context").await.unwrap();
}

that's more than a little surprising.

Builder::finish() should flush the inner writer, or at the very least, this behaviour should be clearly documented

[danieleades]

looking through the docs, the Builder::append* methods mention that Builder::finish should be called, but the Builder::finish method itself says "In most situations the into_inner method should be preferred". That seems inconsistent, no?

In the above snippet, does that mean I can use into_inner().await?.close().await instead of archive_builder.finish().await (rather than both)?

[Nemo157]

I think it would also pay to update all examples to finish/close the writer. The one in the readme is presumably ok since async-std::fs::File is unbuffered, but if someone were to take that and wrap it into an async-std::io::BufWriter they would encounter the same sort of issue with their writes not making it to disk. Adding a finish/close call to it, while not strictly necessary, should have no runtime cost and help with new users learning they need to call close.

[danieleades]

hi @dignifiedquire could i get a bit of clarification on the questions above? I'm happy to take a run at this, but I want to make sure I understand the preferred solution

[dignifiedquire]

Lets go with updating examples & docs for now. I agree it is surprising, but I think I'd like to keep the api as it is for now. We could add a call finish_and_close() or sth like that, as a convenience, for making sure everything is done, thoughts?

[piegamesde]

I agree that finish probably should call flush on the inner writer, but why close it?