Can't read to EOF on BufReader returned from `get_descriptor()`

[allisonkarlitskaya]

If you call ImageProxy::get_descriptor() and try to call .read_to_end() on the returned impl AsyncBufRead then you'll stall forever.

    let (mut blob_reader, driver) = self.proxy.get_descriptor(&self.img, &descriptor).await?;
    blob_reader.read_to_end(&mut raw_config).await?;  // <- gets stuck here
    driver.await?;

Looking at the example code in the repository, you can see that something like this works, however:

    let (mut blob_reader, driver) = self.proxy.get_descriptor(&self.img, &descriptor).await?;
    let mut raw_config = vec![];
    let (a, b) = tokio::join!(blob_reader.read_to_end(&mut raw_config), driver);
    a?;
    b?;

It would be cool if either:

• the 'naive' approach worked; or
• what you need to do with the return values got documented as part of the function docs

[cgwalters]

This is related to ostreedev/ostree-rs-ext#657

[cgwalters]

We chatted about this realtime but the basic issue right now is that the proxy is the thing doing checksum verification, and I didn't want to present "checksum mismatch" as "final error on read".

So today driver represents "final work apart from reading the raw data".

The other thing one needs to understand is that in async rust futures don't make progress unless polled and when doing

blob_reader.read_to_end(&mut raw_config).await?;
driver.await?;

The driver async task isn't polled until the first completes, but join will poll both to ensure they both progress.

[allisonkarlitskaya]

We chatted about this realtime but the basic issue right now is that the proxy is the thing doing checksum verification, and I didn't want to present "checksum mismatch" as "final error on read".

That makes a lot of sense, which is why I make sure to await driver and check for an error before actually using the data... but having a Read impl that deadlocks when you try to read it to the end unless you're also polling on another awaitable is definitely a bit on the weird side...

[allisonkarlitskaya]

fwiw, I'd find the "final error on read" approach to be quite reasonable. I guess your reasons there are something like not wanting to have to shoe-horn the error into the standard io error type? I think I read somewhere that you can optionally chain additional errors there somehow...?

[allisonkarlitskaya]

I ran into this again and completely forgot that I had already opened an issue about it.

But since I forgot about it and tried to pin it down again: here's a fully-working reproducer:

use anyhow::Result;
use oci_spec::image::ImageManifest;
use tokio::io::AsyncReadExt;

async fn pull_image(image: &str) -> Result<()> {
    let proxy = containers_image_proxy::ImageProxy::new().await?;
    let img = proxy.open_image(image).await?;
    let (_manifest_digest, raw_manifest) = proxy.fetch_manifest_raw_oci(&img).await?;

    let manifest = ImageManifest::from_reader(raw_manifest.as_slice())?;
    println!("got manifest {manifest:?}");
    for layer_descriptor in manifest.layers() {
        let (mut blob_reader, driver) = proxy.get_descriptor(&img, layer_descriptor).await?;

        // Both of these code paths stall in similar ways
        if true {
            // This loop successfully ends...
            let mut size = layer_descriptor.size() as usize;
            while size > 0 {
                let mut discard = vec![0; size];
                println!("reading {size} bytes");
                let n = blob_reader.read(&mut discard).await?;
                println!("done {n}!");
                size -= n;
            }

            // But we get stuck here...
            println!("reading 1 extra byte");
            let mut one_byte = [0];
            let n = blob_reader.read(&mut one_byte).await?;
            println!("done {n}!");
        } else {
            let mut discard = vec![];
            println!("read layer to end {layer_descriptor:?}");
            blob_reader.read_to_end(&mut discard).await?;
            println!("done!");
        }

        println!("awaiting driver");
        driver.await?;
        println!("done!")
    }

    Ok(())
}

fn main() -> Result<()> {
    let runtime = tokio::runtime::Builder::new_current_thread()
        .enable_all()
        .build()
        .expect("Failed to build tokio runtime");

    runtime.block_on(async move { pull_image("docker://docker.io/alpine").await })?;
    Ok(())
}

[allisonkarlitskaya]

...and something like the best possible workaround, I think:

        let mut blob_reader = blob_reader.take(layer_descriptor.size());

[allisonkarlitskaya]

I can also confirm that this is still failing as of the latest git version (I added a git ref in the Cargo.toml of the above example):

[package]
name = "skopeo-reproducer"
version = "0.1.0"
edition = "2024"

[dependencies]
anyhow = "1.0.97"
containers-image-proxy = { git = "https://github.com/containers/containers-image-proxy-rs", rev = "fbe78a9a90e06343e9bfdd5559161f1da57ac5a0" }
oci-spec = "0.7.1"
tokio = "1.44.1"

[allisonkarlitskaya]

We chatted about this realtime but the basic issue right now is that the proxy is the thing doing checksum verification, and I didn't want to present "checksum mismatch" as "final error on read".

Indeed. This even seems to come from skopeo itself: https://github.com/containers/skopeo/blob/ddc2848b666f697ac4eaead47f3ffc618f745789/cmd/skopeo/proxy.go#L54

Note that the pipe will not be closed by the server until
the client has invoked FinishPipe. This is to ensure
that the client checks for errors. For example, GetBlob
performs digest (e.g. sha256) verification and this must
be checked after all data has been written.

[cgwalters]

    let mut blob_reader = blob_reader.take(layer_descriptor.size());

Agreed! That's a good suggestion. Done in

#77