TestStreamingWriteLayer: value not computed until stream is consumed

[imjasonh]

Saw this flake in CI: https://github.com/google/go-containerregistry/runs/4908636177?check_suite_focus=true#step:4:41

--- FAIL: TestStreamingWriteLayer (0.01s)
    write_test.go:531: (Path).AppendImage() = error writing layer: error getting final digest of layer: value not computed until stream is consumed

This test was added recently in #1226

@ben-krieger any idea what might cause this to happen? 🤔

[ben-krieger]

@imjasonh No, I don't have any immediate ideas, but I'll take a look.

I can produce it locally by running tests locally with Go 1.17.2 and running the test 1000 times consecutively.

One thing I've noted is that it fails about 1% of the time with the race detector off and < 0.1% of the time with the race detector on.

[ben-krieger]

This appears to fix it:

diff --git a/pkg/v1/layout/write.go b/pkg/v1/layout/write.go
index 4c580e25..7c54e5f5 100644
--- a/pkg/v1/layout/write.go
+++ b/pkg/v1/layout/write.go
@@ -228,7 +228,7 @@ func (l Path) WriteBlob(hash v1.Hash, r io.ReadCloser) error {
        return l.writeBlob(hash, -1, r, nil)
 }
 
-func (l Path) writeBlob(hash v1.Hash, size int64, r io.Reader, renamer func() (v1.Hash, error)) error {
+func (l Path) writeBlob(hash v1.Hash, size int64, rc io.ReadCloser, renamer func() (v1.Hash, error)) error {
        if hash.Hex == "" && renamer == nil {
                panic("writeBlob called an invalid hash and no renamer")
        }
@@ -264,12 +264,21 @@ func (l Path) writeBlob(hash v1.Hash, size int64, r io.Reader, renamer func() (v
        defer w.Close()
 
        // Write to file and exit if not renaming
-       if n, err := io.Copy(w, r); err != nil || renamer == nil {
+       if n, err := io.Copy(w, rc); err != nil || renamer == nil {
                return err
        } else if size != -1 && n != size {
                return fmt.Errorf("expected blob size %d, but only wrote %d", size, n)
        }
 
+       // Always close reader before renaming, since Close computes the digest in
+       // the case of streaming layers. If Close is not called explicitly, it will
+       // occur in a goroutine that is not guaranteed to succeed before renamer is
+       // called. When renamer is the layer's Digest method, it can return
+       // ErrNotComputed.
+       if err := rc.Close(); err != nil {
+               return err
+       }
+
        // Always close file before renaming
        if err := w.Close(); err != nil {
                return err

EDIT: I've run the test 52,000 times in a row successfully, so I'm very confident now.