Do not fetch recursive pins from pinner unnecessarily #7883
Conversation
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().
| @@ -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) | |||
There was a problem hiding this comment.
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().
Ah, I see. Unfortunately, that doesn't really fix the issue as we're not guaranteed to see that the context has been canceled immediately.
There was a problem hiding this comment.
Yes, you are correct that if the caller abandons waiting, we may still try to deliver other results before delivering the error... in which case the goroutine is back to blocking on writing to the channel. The idea is that this may help if the caller gives up waiting (context times out, or is canceled) due to waiting too long for results.
Since this does not fix actually prevent leaking the goroutine, I can remove the buffering, and instead add a comment stating that the caller must keep reading results until the channel is closed to prevent leaking the goroutine. Or, provide an implementation similar to ipfs/interface-go-ipfs-core#62
There was a problem hiding this comment.
I'm fine keeping the buffering just to be nice. I'd also add a comment.
Let's punt on the interface changes for now.
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().