Merge pull request #73 from WKBae/close_object

Close Reader & Writer of EncodedObject after use
go-git · May 24, 2020 · 8019144 · 8019144
2 parents e7f5448 + 1019994
commit 8019144
Show file tree

Hide file tree

Showing 8 changed files with 55 additions and 14 deletions.
diff --git a/plumbing/format/packfile/diff_delta.go b/plumbing/format/packfile/diff_delta.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/utils/ioutil"
 )
 
 // See https://github.com/jelmer/dulwich/blob/master/dulwich/pack.py and
@@ -27,17 +28,20 @@ func GetDelta(base, target plumbing.EncodedObject) (plumbing.EncodedObject, erro
 	return getDelta(new(deltaIndex), base, target)
 }
 
-func getDelta(index *deltaIndex, base, target plumbing.EncodedObject) (plumbing.EncodedObject, error) {
+func getDelta(index *deltaIndex, base, target plumbing.EncodedObject) (o plumbing.EncodedObject, err error) {
 	br, err := base.Reader()
 	if err != nil {
 		return nil, err
 	}
-	defer br.Close()
+
+	defer ioutil.CheckClose(br, &err)
+
 	tr, err := target.Reader()
 	if err != nil {
 		return nil, err
 	}
-	defer tr.Close()
+
+	defer ioutil.CheckClose(tr, &err)
 
 	bb := bufPool.Get().(*bytes.Buffer)
 	defer bufPool.Put(bb)

diff --git a/plumbing/format/packfile/encoder.go b/plumbing/format/packfile/encoder.go
@@ -9,6 +9,7 @@ import (
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/storer"
 	"github.com/go-git/go-git/v5/utils/binary"
+	"github.com/go-git/go-git/v5/utils/ioutil"
 )
 
 // Encoder gets the data from the storage and write it into the writer in PACK
@@ -80,7 +81,7 @@ func (e *Encoder) head(numEntries int) error {
 	)
 }
 
-func (e *Encoder) entry(o *ObjectToPack) error {
+func (e *Encoder) entry(o *ObjectToPack) (err error) {
 	if o.WantWrite() {
 		// A cycle exists in this delta chain. This should only occur if a
 		// selected object representation disappeared during writing
@@ -119,17 +120,22 @@ func (e *Encoder) entry(o *ObjectToPack) error {
 	}
 
 	e.zw.Reset(e.w)
+
+	defer ioutil.CheckClose(e.zw, &err)
+
 	or, err := o.Object.Reader()
 	if err != nil {
 		return err
 	}
 
+	defer ioutil.CheckClose(or, &err)
+
 	_, err = io.Copy(e.zw, or)
 	if err != nil {
 		return err
 	}
 
-	return e.zw.Close()
+	return nil
 }
 
 func (e *Encoder) writeBaseIfDelta(o *ObjectToPack) error {

diff --git a/plumbing/format/packfile/encoder_test.go b/plumbing/format/packfile/encoder_test.go
@@ -287,6 +287,12 @@ func objectsEqual(c *C, o1, o2 plumbing.EncodedObject) {
 	c.Assert(err, IsNil)
 
 	c.Assert(bytes.Compare(b1, b2), Equals, 0)
+
+	err = r2.Close()
+	c.Assert(err, IsNil)
+
+	err = r1.Close()
+	c.Assert(err, IsNil)
 }
 
 func packfileFromReader(c *C, buf *bytes.Buffer) (*Packfile, func()) {

diff --git a/plumbing/format/packfile/packfile.go b/plumbing/format/packfile/packfile.go
@@ -10,6 +10,7 @@ import (
 	"github.com/go-git/go-git/v5/plumbing/cache"
 	"github.com/go-git/go-git/v5/plumbing/format/idxfile"
 	"github.com/go-git/go-git/v5/plumbing/storer"
+	"github.com/go-git/go-git/v5/utils/ioutil"
 )
 
 var (
@@ -307,12 +308,14 @@ func (p *Packfile) getNextMemoryObject(h *ObjectHeader) (plumbing.EncodedObject,
 	return obj, nil
 }
 
-func (p *Packfile) fillRegularObjectContent(obj plumbing.EncodedObject) error {
+func (p *Packfile) fillRegularObjectContent(obj plumbing.EncodedObject) (err error) {
 	w, err := obj.Writer()
 	if err != nil {
 		return err
 	}
 
+	defer ioutil.CheckClose(w, &err)
+
 	_, _, err = p.s.NextObject(w)
 	p.cachePut(obj)
 

diff --git a/plumbing/format/packfile/parser.go b/plumbing/format/packfile/parser.go
@@ -4,11 +4,12 @@ import (
 	"bytes"
 	"errors"
 	"io"
-	"io/ioutil"
+	stdioutil "io/ioutil"
 
 	"github.com/go-git/go-git/v5/plumbing"
 	"github.com/go-git/go-git/v5/plumbing/cache"
 	"github.com/go-git/go-git/v5/plumbing/storer"
+	"github.com/go-git/go-git/v5/utils/ioutil"
 )
 
 var (
@@ -283,7 +284,7 @@ func (p *Parser) resolveDeltas() error {
 
 		if !obj.IsDelta() && len(obj.Children) > 0 {
 			for _, child := range obj.Children {
-				if err := p.resolveObject(ioutil.Discard, child, content); err != nil {
+				if err := p.resolveObject(stdioutil.Discard, child, content); err != nil {
 					return err
 				}
 			}
@@ -298,7 +299,7 @@ func (p *Parser) resolveDeltas() error {
 	return nil
 }
 
-func (p *Parser) get(o *objectInfo, buf *bytes.Buffer) error {
+func (p *Parser) get(o *objectInfo, buf *bytes.Buffer) (err error) {
 	if !o.ExternalRef { // skip cache check for placeholder parents
 		b, ok := p.cache.Get(o.Offset)
 		if ok {
@@ -310,17 +311,21 @@ func (p *Parser) get(o *objectInfo, buf *bytes.Buffer) error {
 	// If it's not on the cache and is not a delta we can try to find it in the
 	// storage, if there's one. External refs must enter here.
 	if p.storage != nil && !o.Type.IsDelta() {
-		e, err := p.storage.EncodedObject(plumbing.AnyObject, o.SHA1)
+		var e plumbing.EncodedObject
+		e, err = p.storage.EncodedObject(plumbing.AnyObject, o.SHA1)
 		if err != nil {
 			return err
 		}
 		o.Type = e.Type()
 
-		r, err := e.Reader()
+		var r io.ReadCloser
+		r, err = e.Reader()
 		if err != nil {
 			return err
 		}
 
+		defer ioutil.CheckClose(r, &err)
+
 		_, err = buf.ReadFrom(io.LimitReader(r, e.Size()))
 		return err
 	}

diff --git a/plumbing/format/packfile/patch_delta.go b/plumbing/format/packfile/patch_delta.go
@@ -6,6 +6,7 @@ import (
 	"io"
 
 	"github.com/go-git/go-git/v5/plumbing"
+	"github.com/go-git/go-git/v5/utils/ioutil"
 )
 
 // See https://github.com/git/git/blob/49fa3dc76179e04b0833542fa52d0f287a4955ac/delta.h
@@ -16,17 +17,21 @@ import (
 const deltaSizeMin = 4
 
 // ApplyDelta writes to target the result of applying the modification deltas in delta to base.
-func ApplyDelta(target, base plumbing.EncodedObject, delta []byte) error {
+func ApplyDelta(target, base plumbing.EncodedObject, delta []byte) (err error) {
 	r, err := base.Reader()
 	if err != nil {
 		return err
 	}
 
+	defer ioutil.CheckClose(r, &err)
+
 	w, err := target.Writer()
 	if err != nil {
 		return err
 	}
 
+	defer ioutil.CheckClose(w, &err)
+
 	buf := bufPool.Get().(*bytes.Buffer)
 	defer bufPool.Put(buf)
 	buf.Reset()

diff --git a/storage/filesystem/object.go b/storage/filesystem/object.go
@@ -408,6 +408,8 @@ func (s *ObjectStorage) getFromUnpacked(h plumbing.Hash) (obj plumbing.EncodedOb
 		return nil, err
 	}
 
+	defer ioutil.CheckClose(w, &err)
+
 	s.objectCache.Put(obj)
 
 	_, err = io.Copy(w, r)

diff --git a/storage/test/storage_suite.go b/storage/test/storage_suite.go
@@ -494,12 +494,12 @@ func objectEquals(a plumbing.EncodedObject, b plumbing.EncodedObject) error {
 
 	ra, err := a.Reader()
 	if err != nil {
-		return fmt.Errorf("can't get reader on b: %q", err)
+		return fmt.Errorf("can't get reader on a: %q", err)
 	}
 
 	rb, err := b.Reader()
 	if err != nil {
-		return fmt.Errorf("can't get reader on a: %q", err)
+		return fmt.Errorf("can't get reader on b: %q", err)
 	}
 
 	ca, err := ioutil.ReadAll(ra)
@@ -516,5 +516,15 @@ func objectEquals(a plumbing.EncodedObject, b plumbing.EncodedObject) error {
 		return errors.New("content does not match")
 	}
 
+	err = rb.Close()
+	if err != nil {
+		return fmt.Errorf("can't close reader on b: %q", err)
+	}
+
+	err = ra.Close()
+	if err != nil {
+		return fmt.Errorf("can't close reader on a: %q", err)
+	}
+
 	return nil
 }