From 07aa00dfa69e81a7dd5623d82ed130a46dd9c188 Mon Sep 17 00:00:00 2001 From: gammazero Date: Wed, 27 Jan 2021 10:17:53 -0800 Subject: [PATCH 1/2] Do not fetch recursive pins from pinner unnecessarily When fetching all pins, the recursive pins are fetched from the pinner two times. The second fetch is unnecessary and copies all recursive pins into a slice again. Additionally, the output channel is now buffered. This allows the goroutine to exit in the case the pinner returns an error and there is no reader for the output channel. This might be possible if a canceled context causes the caller to abandon waiting to read the output of Ls(). --- core/coreapi/pin.go | 29 +++++++++++++---------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/core/coreapi/pin.go b/core/coreapi/pin.go index 3b6ae2d9395..160db2c047c 100644 --- a/core/coreapi/pin.go +++ b/core/coreapi/pin.go @@ -220,7 +220,7 @@ func (p *pinInfo) Err() error { // pinLsAll is an internal function for returning a list of pins func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreiface.Pin { - out := make(chan coreiface.Pin) + out := make(chan coreiface.Pin, 1) keys := cid.NewSet() @@ -249,37 +249,34 @@ func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreifac go func() { defer close(out) + var dkeys, rkeys []cid.Cid + var err error if typeStr == "recursive" || typeStr == "all" { - rkeys, err := api.pinning.RecursiveKeys(ctx) + rkeys, err = api.pinning.RecursiveKeys(ctx) if err != nil { out <- &pinInfo{err: err} return } - if err := AddToResultKeys(rkeys, "recursive"); err != nil { + if err = AddToResultKeys(rkeys, "recursive"); err != nil { out <- &pinInfo{err: err} return } } if typeStr == "direct" || typeStr == "all" { - dkeys, err := api.pinning.DirectKeys(ctx) + dkeys, err = api.pinning.DirectKeys(ctx) if err != nil { out <- &pinInfo{err: err} return } - if err := AddToResultKeys(dkeys, "direct"); err != nil { + if err = AddToResultKeys(dkeys, "direct"); err != nil { out <- &pinInfo{err: err} return } } if typeStr == "all" { set := cid.NewSet() - rkeys, err := api.pinning.RecursiveKeys(ctx) - if err != nil { - out <- &pinInfo{err: err} - return - } for _, k := range rkeys { - err := merkledag.Walk( + err = merkledag.Walk( ctx, merkledag.GetLinksWithDAG(api.dag), k, set.Visit, merkledag.SkipRoot(), merkledag.Concurrent(), @@ -289,7 +286,7 @@ func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreifac return } } - if err := AddToResultKeys(set.Keys(), "indirect"); err != nil { + if err = AddToResultKeys(set.Keys(), "indirect"); err != nil { out <- &pinInfo{err: err} return } @@ -298,14 +295,14 @@ func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreifac // We need to first visit the direct pins that have priority // without emitting them - dkeys, err := api.pinning.DirectKeys(ctx) + dkeys, err = api.pinning.DirectKeys(ctx) if err != nil { out <- &pinInfo{err: err} return } VisitKeys(dkeys) - rkeys, err := api.pinning.RecursiveKeys(ctx) + rkeys, err = api.pinning.RecursiveKeys(ctx) if err != nil { out <- &pinInfo{err: err} return @@ -314,7 +311,7 @@ func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreifac set := cid.NewSet() for _, k := range rkeys { - err := merkledag.Walk( + err = merkledag.Walk( ctx, merkledag.GetLinksWithDAG(api.dag), k, set.Visit, merkledag.SkipRoot(), merkledag.Concurrent(), @@ -324,7 +321,7 @@ func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreifac return } } - if err := AddToResultKeys(set.Keys(), "indirect"); err != nil { + if err = AddToResultKeys(set.Keys(), "indirect"); err != nil { out <- &pinInfo{err: err} return } From fb55f09882c0b0dcf395645b56d34be3840aceba Mon Sep 17 00:00:00 2001 From: gammazero Date: Mon, 29 Mar 2021 15:11:38 -0700 Subject: [PATCH 2/2] Add comment about reading until channel closed --- core/coreapi/pin.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/coreapi/pin.go b/core/coreapi/pin.go index 160db2c047c..b5db63d35a1 100644 --- a/core/coreapi/pin.go +++ b/core/coreapi/pin.go @@ -219,6 +219,9 @@ func (p *pinInfo) Err() error { } // pinLsAll is an internal function for returning a list of pins +// +// The caller must keep reading results until the channel is closed to prevent +// leaking the goroutine that is fetching pins. func (api *PinAPI) pinLsAll(ctx context.Context, typeStr string) <-chan coreiface.Pin { out := make(chan coreiface.Pin, 1)