client: Client.ImageSave: close reader on context cancellation

Use a cancelReadCloser to automatically close the reader when the context
is cancelled. Consumers are still recommended to manually close the reader,
but the cancelReadCloser makes the Close idempotent.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

client/image_save.go

@@ -11,7 +11,8 @@ type ImageSaveResult interface {
 }
 
 // ImageSave retrieves one or more images from the docker host as an
-// [ImageSaveResult].
+// [ImageSaveResult]. Callers should close the reader, but the underlying
+// [io.ReadCloser] is automatically closed if the context is canceled,
 //
 // Platforms is an optional parameter that specifies the platforms to save
 // from the image. Passing a platform only has an effect if the input image
@@ -44,30 +45,15 @@ func (cli *Client) ImageSave(ctx context.Context, imageIDs []string, saveOpts ..
 		return nil, err
 	}
 	return &imageSaveResult{
-		body: resp.Body,
+		ReadCloser: newCancelReadCloser(ctx, resp.Body),
 	}, nil
 }
 
 type imageSaveResult struct {
-	// body must be closed to avoid a resource leak
-	body io.ReadCloser
+	io.ReadCloser
 }
 
 var (
 	_ io.ReadCloser   = (*imageSaveResult)(nil)
 	_ ImageSaveResult = (*imageSaveResult)(nil)
 )
-
-func (r *imageSaveResult) Read(p []byte) (int, error) {
-	if r == nil || r.body == nil {
-		return 0, io.EOF
-	}
-	return r.body.Read(p)
-}
-
-func (r *imageSaveResult) Close() error {
-	if r == nil || r.body == nil {
-		return nil
-	}
-	return r.body.Close()
-}

client/image_save_opts.go

@@ -16,8 +16,11 @@ func (f imageSaveOptionFunc) Apply(o *imageSaveOpts) error {
 	return f(o)
 }
 
-// ImageSaveWithPlatforms sets the platforms to be saved from the image.
+// ImageSaveWithPlatforms sets the platforms to be saved from the image. It
+// produces an error if platforms are already set. This option only has an
+// effect if the input image is a multi-platform image.
 func ImageSaveWithPlatforms(platforms ...ocispec.Platform) ImageSaveOption {
+	// TODO(thaJeztah): verify the GoDoc; do we produce an error for a single-platform image without the given platform?
 	return imageSaveOptionFunc(func(opt *imageSaveOpts) error {
 		if opt.apiOptions.Platforms != nil {
 			return fmt.Errorf("platforms already set to %v", opt.apiOptions.Platforms)

client/image_save_test.go

@@ -1,7 +1,6 @@
 package client
 
 import (
-	"context"
 	"io"
 	"net/http"
 	"net/url"
@@ -17,7 +16,7 @@ func TestImageSaveError(t *testing.T) {
 	client, err := New(WithMockClient(errorMock(http.StatusInternalServerError, "Server error")))
 	assert.NilError(t, err)
 	armv64 := ocispec.Platform{Architecture: "arm64", OS: "linux", Variant: "v8"}
-	_, err = client.ImageSave(context.Background(), []string{"nothing"}, ImageSaveWithPlatforms(armv64))
+	_, err = client.ImageSave(t.Context(), []string{"nothing"}, ImageSaveWithPlatforms(armv64))
 	assert.Check(t, is.ErrorType(err, cerrdefs.IsInternal))
 }
 
@@ -69,7 +68,7 @@ func TestImageSave(t *testing.T) {
 				return mockResponse(http.StatusOK, nil, expectedOutput)(req)
 			}))
 			assert.NilError(t, err)
-			resp, err := client.ImageSave(context.Background(), []string{"image_id1", "image_id2"}, tc.options...)
+			resp, err := client.ImageSave(t.Context(), []string{"image_id1", "image_id2"}, tc.options...)
 			assert.NilError(t, err)
 			defer func() {
 				assert.NilError(t, resp.Close())

integration/build/build_userns_linux_test.go

@@ -75,7 +75,7 @@ func TestBuildUserNamespaceValidateCapabilitiesAreV2(t *testing.T) {
 
 	reader, err := clientUserRemap.ImageSave(ctx, []string{imageTag})
 	assert.NilError(t, err, "failed to download capabilities image")
-	defer reader.Close()
+	defer func() { _ = reader.Close() }()
 
 	tar, err := os.Create(filepath.Join(tmpDir, "image.tar"))
 	assert.NilError(t, err, "failed to create image tar file")

integration/daemon/migration_test.go

@@ -130,8 +130,8 @@ func TestMigrateSaveLoad(t *testing.T) {
 	rdr, err := apiClient.ImageSave(ctx, []string{"busybox:latest"})
 	assert.NilError(t, err)
 	buf := bytes.NewBuffer(nil)
-	io.Copy(buf, rdr)
-	rdr.Close()
+	_, _ = io.Copy(buf, rdr)
+	defer func() { _ = rdr.Close() }()
 
 	// Delete all images
 	list, err := apiClient.ImageList(ctx, client.ImageListOptions{})

integration/image/save_test.go

@@ -44,7 +44,7 @@ func tarIndexFS(t *testing.T, rdr io.Reader) fs.FS {
 	assert.NilError(t, err)
 
 	// Do not close at the end of this function otherwise the indexer won't work
-	t.Cleanup(func() { f.Close() })
+	t.Cleanup(func() { _ = f.Close() })
 
 	_, err = io.Copy(f, rdr)
 	assert.NilError(t, err)
@@ -64,6 +64,7 @@ func TestSaveCheckTimes(t *testing.T) {
 
 	rdr, err := apiClient.ImageSave(ctx, []string{repoName})
 	assert.NilError(t, err)
+	defer func() { _ = rdr.Close() }()
 
 	created, err := time.Parse(time.RFC3339, img.Created)
 	assert.NilError(t, err)
@@ -135,7 +136,7 @@ func TestSaveOCI(t *testing.T) {
 
 			rdr, err := apiClient.ImageSave(ctx, []string{tc.image})
 			assert.NilError(t, err)
-			defer rdr.Close()
+			defer func() { _ = rdr.Close() }()
 
 			tarfs := tarIndexFS(t, rdr)
 
@@ -171,7 +172,7 @@ func TestSaveOCI(t *testing.T) {
 					assert.NilError(t, err)
 
 					layerDigest, err := testutil.UncompressedTarDigest(f)
-					f.Close()
+					_ = f.Close()
 
 					assert.NilError(t, err)
 
@@ -426,7 +427,7 @@ func TestSaveRepoWithMultipleImages(t *testing.T) {
 
 	rdr, err := apiClient.ImageSave(ctx, []string{repoName, "busybox:latest"})
 	assert.NilError(t, err)
-	defer rdr.Close()
+	defer func() { _ = rdr.Close() }()
 
 	tarfs := tarIndexFS(t, rdr)
 
@@ -483,7 +484,7 @@ RUN touch /opt/a/b/c && chown user:user /opt/a/b/c`
 
 	rdr, err := apiClient.ImageSave(ctx, []string{imgID})
 	assert.NilError(t, err)
-	defer rdr.Close()
+	defer func() { _ = rdr.Close() }()
 
 	tarfs := tarIndexFS(t, rdr)
 

integration/plugin/authz/authz_plugin_test.go

@@ -425,17 +425,17 @@ func TestAuthzPluginEnsureContainerCopyToFrom(t *testing.T) {
 }
 
 func imageSave(ctx context.Context, apiClient client.APIClient, path, imgRef string) error {
-	responseReader, err := apiClient.ImageSave(ctx, []string{imgRef})
+	resp, err := apiClient.ImageSave(ctx, []string{imgRef})
 	if err != nil {
 		return err
 	}
-	defer responseReader.Close()
+	defer func() { _ = resp.Close() }()
 	file, err := os.Create(path)
 	if err != nil {
 		return err
 	}
-	defer file.Close()
-	_, err = io.Copy(file, responseReader)
+	_, err = io.Copy(file, resp)
+	_ = file.Close()
 	return err
 }
 

internal/testutil/daemon/daemon.go

@@ -879,7 +879,7 @@ func (d *Daemon) LoadImage(ctx context.Context, t testing.TB, img string) {
 
 	reader, err := clientHost.ImageSave(ctx, []string{img})
 	assert.NilError(t, err, "[%s] failed to download %s", d.id, img)
-	defer reader.Close()
+	defer func() { _ = reader.Close() }()
 
 	c := d.NewClientT(t)
 	defer c.Close()

vendor/github.com/moby/moby/client/image_save.go

@@ -11,7 +11,8 @@ type ImageSaveResult interface {
 }
 
 // ImageSave retrieves one or more images from the docker host as an
-// [ImageSaveResult].
+// [ImageSaveResult]. Callers should close the reader, but the underlying
+// [io.ReadCloser] is automatically closed if the context is canceled,
 //
 // Platforms is an optional parameter that specifies the platforms to save
 // from the image. Passing a platform only has an effect if the input image
@@ -44,30 +45,15 @@ func (cli *Client) ImageSave(ctx context.Context, imageIDs []string, saveOpts ..
 		return nil, err
 	}
 	return &imageSaveResult{
-		body: resp.Body,
+		ReadCloser: newCancelReadCloser(ctx, resp.Body),
 	}, nil
 }
 
 type imageSaveResult struct {
-	// body must be closed to avoid a resource leak
-	body io.ReadCloser
+	io.ReadCloser
 }
 
 var (
 	_ io.ReadCloser   = (*imageSaveResult)(nil)
 	_ ImageSaveResult = (*imageSaveResult)(nil)
 )
-
-func (r *imageSaveResult) Read(p []byte) (int, error) {
-	if r == nil || r.body == nil {
-		return 0, io.EOF
-	}
-	return r.body.Read(p)
-}
-
-func (r *imageSaveResult) Close() error {
-	if r == nil || r.body == nil {
-		return nil
-	}
-	return r.body.Close()
-}

vendor/github.com/moby/moby/client/image_save_opts.go

@@ -16,8 +16,11 @@ func (f imageSaveOptionFunc) Apply(o *imageSaveOpts) error {
 	return f(o)
 }
 
-// ImageSaveWithPlatforms sets the platforms to be saved from the image.
+// ImageSaveWithPlatforms sets the platforms to be saved from the image. It
+// produces an error if platforms are already set. This option only has an
+// effect if the input image is a multi-platform image.
 func ImageSaveWithPlatforms(platforms ...ocispec.Platform) ImageSaveOption {
+	// TODO(thaJeztah): verify the GoDoc; do we produce an error for a single-platform image without the given platform?
 	return imageSaveOptionFunc(func(opt *imageSaveOpts) error {
 		if opt.apiOptions.Platforms != nil {
 			return fmt.Errorf("platforms already set to %v", opt.apiOptions.Platforms)