[cipd] stop doing weird ownership transfer
R=vadimsh@chromium.org
BUG=
Review-Url: https://codereview.chromium.org/2877643002
diff --git a/cipd/client/cipd/client.go b/cipd/client/cipd/client.go
index 9d2f8bb..08bbfdc 100644
--- a/cipd/client/cipd/client.go
+++ b/cipd/client/cipd/client.go
@@ -1272,13 +1272,9 @@
return err
}
- // Make sure to close the file if paniced before OpenInstance call below.
- owned := false
defer func() {
- if !owned {
- if err := instanceFile.Close(); err != nil {
- logging.Warningf(ctx, "cipd: failed to close the package file - %s", err)
- }
+ if err := instanceFile.Close(); err != nil && err != os.ErrClosed {
+ logging.Warningf(ctx, "cipd: failed to close the package file - %s", err)
}
}()
@@ -1289,14 +1285,8 @@
return err
}
- owned = true // disarm the defer above, instance.Close closes instanceFile.
- defer func() {
- if err := instance.Close(); err != nil {
- logging.Warningf(ctx, "cipd: failed to close the instance - %s", err)
- }
- // Opportunistically clean up trashed files.
- client.doBatchAwareOp(ctx, batchAwareOpCleanupTrash)
- }()
+ // Opportunistically clean up trashed files.
+ defer client.doBatchAwareOp(ctx, batchAwareOpCleanupTrash)
// Deploy it. 'defer' will take care of removing the temp file if needed.
_, err = client.deployer.DeployInstance(ctx, subdir, instance)
diff --git a/cipd/client/cipd/client_test.go b/cipd/client/cipd/client_test.go
index a6a2fe4..023512f 100644
--- a/cipd/client/cipd/client_test.go
+++ b/cipd/client/cipd/client_test.go
@@ -147,7 +147,6 @@
// Open it for reading.
inst, err := local.OpenInstance(ctx, bytes.NewReader(out.Bytes()), "", local.VerifyHash)
So(err, ShouldBeNil)
- defer inst.Close()
Convey("RegisterInstance full flow", func(c C) {
client := mockClient(c, "", []expectedHTTPCall{
@@ -487,13 +486,13 @@
inst := buildInstanceInMemory(ctx, "testing/package", []local.File{
local.NewTestFile("file", "test data", false),
})
- defer inst.Close()
client := mockClientForFetch(c, tempDir, []local.PackageInstance{inst})
Convey("FetchInstance (no cache)", func() {
reader, err := client.FetchInstance(ctx, inst.Pin())
So(err, ShouldBeNil)
+ defer reader.Close() // just in case
// Backed by a temp file.
tmpFile := reader.(deleteOnClose)
@@ -503,7 +502,7 @@
fetched, err := local.OpenInstance(ctx, reader, "", local.VerifyHash)
So(err, ShouldBeNil)
So(fetched.Pin(), ShouldResemble, inst.Pin())
- fetched.Close() // this closes the 'reader' too
+ tmpFile.Close()
// The temp file is gone.
_, err = os.Stat(tmpFile.Name())
@@ -515,6 +514,7 @@
reader, err := client.FetchInstance(ctx, inst.Pin())
So(err, ShouldBeNil)
+ defer reader.Close()
// Backed by a real file.
cachedFile := reader.(*os.File)
@@ -524,7 +524,6 @@
fetched, err := local.OpenInstance(ctx, reader, "", local.VerifyHash)
So(err, ShouldBeNil)
So(fetched.Pin(), ShouldResemble, inst.Pin())
- fetched.Close() // this closes the 'reader' too
// The real file is still there, in the cache.
_, err = os.Stat(cachedFile.Name())
@@ -553,10 +552,10 @@
So(err, ShouldBeNil)
out.Close()
- fetched, err := local.OpenInstanceFile(ctx, tempFile, "", local.VerifyHash)
+ fetched, closer, err := local.OpenInstanceFile(ctx, tempFile, "", local.VerifyHash)
So(err, ShouldBeNil)
+ defer closer()
So(fetched.Pin(), ShouldResemble, inst.Pin())
- fetched.Close()
})
Convey("FetchInstanceTo (with cache)", func() {
@@ -571,10 +570,10 @@
So(err, ShouldBeNil)
out.Close()
- fetched, err := local.OpenInstanceFile(ctx, tempFile, "", local.VerifyHash)
+ fetched, closer, err := local.OpenInstanceFile(ctx, tempFile, "", local.VerifyHash)
So(err, ShouldBeNil)
+ defer closer()
So(fetched.Pin(), ShouldResemble, inst.Pin())
- fetched.Close()
})
Convey("FetchAndDeployInstance works", func() {
@@ -652,11 +651,8 @@
Convey("EnsurePackages full flow", func(c C) {
// Prepare a bunch of packages.
a1 := buildInstanceInMemory(ctx, "pkg/a", []local.File{local.NewTestFile("file a 1", "test data", false)})
- defer a1.Close()
a2 := buildInstanceInMemory(ctx, "pkg/a", []local.File{local.NewTestFile("file a 2", "test data", false)})
- defer a2.Close()
b := buildInstanceInMemory(ctx, "pkg/b", []local.File{local.NewTestFile("file b", "test data", false)})
- defer b.Close()
pil := func(insts ...local.PackageInstance) []local.PackageInstance {
return insts
diff --git a/cipd/client/cipd/local/reader.go b/cipd/client/cipd/local/reader.go
index 1340760..8ead5c0 100644
--- a/cipd/client/cipd/local/reader.go
+++ b/cipd/client/cipd/local/reader.go
@@ -39,9 +39,6 @@
// PackageInstance represents a binary CIPD package file (with manifest inside).
type PackageInstance interface {
- // Close shuts down the package and its data provider.
- Close() error
-
// Pin identifies package name and concreted instance ID of this package file.
Pin() common.Pin
@@ -60,12 +57,6 @@
// If instanceID is not empty and verification mode is VerifyHash,
// OpenInstance will check that package data matches the given instanceID. It
// skips this check if verification mode is SkipHashVerification.
-//
-// If the call succeeds, PackageInstance takes ownership of io.ReadSeeker. If it
-// also implements io.Closer, it will be closed when package.Close() is called.
-//
-// If an error is returned, io.ReadSeeker remains unowned and caller is r
-// esponsible for closing it (if required).
func OpenInstance(ctx context.Context, r io.ReadSeeker, instanceID string, v VerificationMode) (PackageInstance, error) {
out := &packageInstance{data: r}
if err := out.open(instanceID, v); err != nil {
@@ -75,14 +66,20 @@
}
// OpenInstanceFile opens a package instance file on disk.
-func OpenInstanceFile(ctx context.Context, path string, instanceID string, v VerificationMode) (inst PackageInstance, err error) {
+//
+// The caller of this function must call closer() if err != nil to close the
+// underlying file.
+func OpenInstanceFile(ctx context.Context, path string, instanceID string, v VerificationMode) (inst PackageInstance, closer func() error, err error) {
file, err := os.Open(path)
if err != nil {
return
}
inst, err = OpenInstance(ctx, file, instanceID, v)
if err != nil {
+ inst = nil
file.Close()
+ } else {
+ closer = file.Close
}
return
}
@@ -412,20 +409,6 @@
return nil
}
-func (inst *packageInstance) Close() (err error) {
- if inst.data != nil {
- if closer, ok := inst.data.(io.Closer); ok {
- err = closer.Close()
- }
- inst.data = nil
- }
- inst.instanceID = ""
- inst.zip = nil
- inst.files = []File{}
- inst.manifest = Manifest{}
- return
-}
-
func (inst *packageInstance) Pin() common.Pin {
return common.Pin{
PackageName: inst.manifest.PackageName,
diff --git a/cipd/client/cipd/local/reader_test.go b/cipd/client/cipd/local/reader_test.go
index 7ad2b8f..d73688c 100644
--- a/cipd/client/cipd/local/reader_test.go
+++ b/cipd/client/cipd/local/reader_test.go
@@ -64,9 +64,6 @@
// Open it.
inst, err := OpenInstance(ctx, bytes.NewReader(out.Bytes()), "", VerifyHash)
- if inst != nil {
- defer inst.Close()
- }
So(inst, ShouldNotBeNil)
So(err, ShouldBeNil)
So(inst.Pin(), ShouldResemble, Pin{"testing", "23f2c4900785ac8faa2f38e473925b840e574ccc"})
@@ -107,7 +104,6 @@
So(err, ShouldBeNil)
So(inst, ShouldNotBeNil)
So(inst.Pin(), ShouldResemble, Pin{"testing", "23f2c4900785ac8faa2f38e473925b840e574ccc"})
- inst.Close()
// Attempt to open it, providing incorrect instance ID.
inst, err = OpenInstance(ctx, source, "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", VerifyHash)
@@ -119,7 +115,6 @@
So(err, ShouldBeNil)
So(inst, ShouldNotBeNil)
So(inst.Pin(), ShouldResemble, Pin{"testing", "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"})
- inst.Close()
})
Convey("OpenInstanceFile works", t, func() {
@@ -139,12 +134,10 @@
tempFile.Close()
// Read the package.
- inst, err := OpenInstanceFile(ctx, tempFilePath, "", VerifyHash)
- if inst != nil {
- defer inst.Close()
- }
- So(inst, ShouldNotBeNil)
+ inst, closer, err := OpenInstanceFile(ctx, tempFilePath, "", VerifyHash)
So(err, ShouldBeNil)
+ defer closer()
+ So(inst, ShouldNotBeNil)
})
Convey("ExtractInstance works", t, func() {
@@ -176,9 +169,6 @@
// Extract files.
inst, err := OpenInstance(ctx, bytes.NewReader(out.Bytes()), "", VerifyHash)
- if inst != nil {
- defer inst.Close()
- }
So(err, ShouldBeNil)
dest := &testDestination{}
err = ExtractInstance(ctx, inst, dest, func(f File) bool {
diff --git a/cipd/client/cli/main.go b/cipd/client/cli/main.go
index 0258a09..be1c65f 100644
--- a/cipd/client/cli/main.go
+++ b/cipd/client/cli/main.go
@@ -1545,11 +1545,11 @@
}
func deployInstanceFile(ctx context.Context, root string, instanceFile string) (common.Pin, error) {
- inst, err := local.OpenInstanceFile(ctx, instanceFile, "", local.VerifyHash)
+ inst, closer, err := local.OpenInstanceFile(ctx, instanceFile, "", local.VerifyHash)
if err != nil {
return common.Pin{}, err
}
- defer inst.Close()
+ defer closer()
inspectInstance(ctx, inst, false)
d := local.NewDeployer(root)
@@ -1627,12 +1627,12 @@
// the hash.
out.Close()
ok = true
- inst, err := local.OpenInstanceFile(ctx, instanceFile, pin.InstanceID, local.SkipHashVerification)
+ inst, closer, err := local.OpenInstanceFile(ctx, instanceFile, pin.InstanceID, local.SkipHashVerification)
if err != nil {
os.Remove(instanceFile)
return common.Pin{}, err
}
- defer inst.Close()
+ defer closer()
inspectInstance(ctx, inst, false)
return inst.Pin(), nil
}
@@ -1667,11 +1667,11 @@
}
func inspectInstanceFile(ctx context.Context, instanceFile string, listFiles bool) (common.Pin, error) {
- inst, err := local.OpenInstanceFile(ctx, instanceFile, "", local.VerifyHash)
+ inst, closer, err := local.OpenInstanceFile(ctx, instanceFile, "", local.VerifyHash)
if err != nil {
return common.Pin{}, err
}
- defer inst.Close()
+ defer closer()
inspectInstance(ctx, inst, listFiles)
return inst.Pin(), nil
}
@@ -1752,11 +1752,11 @@
}
func registerInstanceFile(ctx context.Context, instanceFile string, opts *registerOpts) (common.Pin, error) {
- inst, err := local.OpenInstanceFile(ctx, instanceFile, "", local.VerifyHash)
+ inst, closer, err := local.OpenInstanceFile(ctx, instanceFile, "", local.VerifyHash)
if err != nil {
return common.Pin{}, err
}
- defer inst.Close()
+ defer closer()
client, err := opts.clientOptions.makeCipdClient(ctx, "")
if err != nil {
return common.Pin{}, err
