Close state leaks from the NewResourceOpener code

This code path is a bit trickier to close than the other handler.
modongsongrio · Dec 15, 2016 · 1390f96 · 1390f96
1 parent 9106c8d
commit 1390f96
Show file tree

Hide file tree

Showing 3 changed files with 71 additions and 6 deletions.
diff --git a/resource/opened.go b/resource/opened.go
@@ -7,12 +7,46 @@ package resource
 
 import (
 	"io"
+	"strings"
+
+	"github.com/juju/errors"
 )
 
+type multiError []error
+
+func (m multiError) Error() string {
+	messages := make([]string, len(m))
+	for i, err := range m {
+		messages[i] = err.Error()
+	}
+	return strings.Join(messages, ", and also ")
+}
+
+// CombineErrors converts a set of errors (which might be nil) into
+// one. If there are no errors, this returns an untyped nil, and if
+// there's one error it's passed through directly.
+func CombineErrors(errs ...error) error {
+	merr := make(multiError, 0, len(errs))
+	for _, err := range errs {
+		if err != nil {
+			merr = append(merr, err)
+		}
+	}
+	if len(merr) == 0 {
+		return nil
+	}
+	if len(merr) == 1 {
+		return merr[0]
+	}
+	return merr
+}
+
 // Opened provides both the resource info and content.
 type Opened struct {
 	Resource
 	io.ReadCloser
+
+	Closer func() error
 }
 
 // Content returns the "content" for the opened resource.
@@ -24,6 +58,15 @@ func (o Opened) Content() Content {
 	}
 }
 
+func (o Opened) Close() error {
+	var err1 error
+	if o.Closer != nil {
+		err1 = errors.Trace(o.Closer())
+	}
+	err2 := errors.Trace(o.ReadCloser.Close())
+	return CombineErrors(err1, err2)
+}
+
 // Opener exposes the functionality for opening a resource.
 type Opener interface {
 	// OpenResource returns an opened resource with a reader that will

diff --git a/resource/resourceadapters/apiserver.go b/resource/resourceadapters/apiserver.go
@@ -48,14 +48,15 @@ func NewApplicationHandler(args apihttp.NewHandlerArgs) http.Handler {
 			if err != nil {
 				return nil, nil, nil, errors.Trace(err)
 			}
+			closer := func() error {
+				return args.Release(st)
+			}
 			resources, err := st.Resources()
 			if err != nil {
+				closer()
 				return nil, nil, nil, errors.Trace(err)
 			}
 
-			closer := func() error {
-				return args.Release(st)
-			}
 			return resources, closer, entity.Tag(), nil
 		},
 	)
@@ -65,6 +66,7 @@ func NewApplicationHandler(args apihttp.NewHandlerArgs) http.Handler {
 func NewDownloadHandler(args apihttp.NewHandlerArgs) http.Handler {
 	extractor := &httpDownloadRequestExtractor{
 		connect: args.Connect,
+		release: args.Release,
 	}
 	deps := internalserver.NewHTTPHandlerDeps(extractor)
 	return internalserver.NewHTTPHandler(deps)
@@ -79,16 +81,27 @@ type stateConnector interface {
 // handle a resource download HTTP request.
 type httpDownloadRequestExtractor struct {
 	connect func(*http.Request) (*corestate.State, corestate.Entity, error)
+	release func(*corestate.State) error
 }
 
 // NewResourceOpener returns a new resource.Opener for the given
 // HTTP request.
-func (ex *httpDownloadRequestExtractor) NewResourceOpener(req *http.Request) (resource.Opener, error) {
+func (ex *httpDownloadRequestExtractor) NewResourceOpener(req *http.Request) (opener resource.Opener, err error) {
 	st, _, err := ex.connect(req)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 
+	closer := func() error {
+		return ex.release(st)
+	}
+
+	defer func() {
+		if err != nil {
+			closer()
+		}
+	}()
+
 	unitTagStr := req.URL.Query().Get(":unit")
 	unitTag, err := names.ParseUnitTag(unitTagStr)
 	if err != nil {
@@ -104,11 +117,12 @@ func (ex *httpDownloadRequestExtractor) NewResourceOpener(req *http.Request) (re
 		return nil, errors.Trace(err)
 	}
 
-	opener := &resourceOpener{
+	opener = &resourceOpener{
 		st:     st,
 		res:    resources,
 		userID: unitTag,
 		unit:   unit,
+		closer: closer,
 	}
 	return opener, nil
 }
diff --git a/resource/resourceadapters/opener.go b/resource/resourceadapters/opener.go
@@ -19,10 +19,17 @@ type resourceOpener struct {
 	res    corestate.Resources
 	userID names.Tag
 	unit   *corestate.Unit
+	closer func() error
 }
 
 // OpenResource implements server.ResourceOpener.
-func (ro *resourceOpener) OpenResource(name string) (resource.Opened, error) {
+func (ro *resourceOpener) OpenResource(name string) (o resource.Opened, err error) {
+	defer func() {
+		if err != nil {
+			ro.closer()
+		}
+	}()
+
 	if ro.unit == nil {
 		return resource.Opened{}, errors.Errorf("missing unit")
 	}
@@ -62,6 +69,7 @@ func (ro *resourceOpener) OpenResource(name string) (resource.Opened, error) {
 	opened := resource.Opened{
 		Resource:   res,
 		ReadCloser: reader,
+		Closer:     ro.closer,
 	}
 	return opened, nil
 }