goroutine leak in openIndexV12 and openTSDBIndex

[liaol]

I identified a goroutine leak with github.com/minio/minio-go/v7.(*Client).GetObject.func1 when use S3 objstore
The root cause is that the Close method is not called on the return value of bucket.Get in these two methods.

pyroscope/pkg/phlaredb/symdb/block_reader.go

Lines 254 to 269 in 81af7c6

	func (r *Reader) openIndexV12(ctx context.Context) error {
	f, err := r.lookupFile(IndexFileName)
	if err != nil {
	return err
	}
	o, err := r.bucket.Get(ctx, f.RelPath)
	if err != nil {
	return err
	}
	b, err := io.ReadAll(o)
	if err != nil {
	return err
	}
	r.index, err = OpenIndex(b)
	return err
	}

pyroscope/pkg/phlaredb/block_querier.go

Lines 2044 to 2079 in 81af7c6

	func (q *singleBlockQuerier) openTSDBIndex(ctx context.Context) error {
	f, err := q.bucket.Get(ctx, block.IndexFilename)
	if err != nil {
	return fmt.Errorf("opening index.tsdb file: %w", err)
	}
	var buf []byte
	var tsdbIndexFile block.File
	for _, mf := range q.meta.Files {
	if mf.RelPath == block.IndexFilename {
	tsdbIndexFile = mf
	break
	}
	}
	if tsdbIndexFile.SizeBytes > 0 {
	// If index size is known beforehand, we can allocate
	// a buffer of the exact size to save some space.
	buf = make([]byte, tsdbIndexFile.SizeBytes)
	_, err = io.ReadFull(f, buf)
	} else {
	// 32KB is the default buf size of io.Copy.
	// It's unlikely that a tsdb index is less than that.
	b := bytes.NewBuffer(make([]byte, 0, 32<<10))
	_, err = io.Copy(b, f)
	buf = b.Bytes()
	}
	if err != nil {
	return fmt.Errorf("reading tsdb index: %w", err)
	}
	q.index, err = index.NewReader(index.RealByteSlice(buf))
	if err != nil {
	return fmt.Errorf("opening tsdb index: %w", err)
	}
	return nil
	}

If confirmed, I can submit a CR.

[korniltsev]

Do you think it's possible to catch with https://github.com/uber-go/goleak ?

I have not looked into the code, but I believe any fixes are welcome!

[liaol]

@korniltsev

I created a simple unit test in pkg/phlaredb/symdb, goleak find a goroutine leak in github.com/minio/minio-go/v7.(*Client).GetObject.func1

Please replace your own s3 config and symdb file when run the test.

package symdb

import (
	"context"
	"testing"
	"time"

	"github.com/grafana/pyroscope/pkg/objstore/client"
	"github.com/grafana/pyroscope/pkg/objstore/providers/s3"
	"github.com/grafana/pyroscope/pkg/phlaredb/block"
	"go.uber.org/goleak"
)

func Test_Reader_Open_v3_leak(t *testing.T) {
	defer goleak.VerifyNone(t)
	cfg := client.Config{
		StoragePrefix: "your prefix",
		StorageBackendConfig: client.StorageBackendConfig{
			Backend: "s3",
			S3: s3.Config{
				SignatureVersion: s3.SignatureVersionV4,
				BucketName:       "your bucket",
				Region:           "your region",
				Endpoint:         "your endpoint",
				BucketLookupType: s3.AutoLookup,
			}}}
	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
	defer cancel()
	b, _ := client.NewBucket(ctx, cfg, "anonymous")
	testBlockMeta1 := &block.Meta{
		Files: []block.File{
			{RelPath: "anonymous/phlaredb/01JX2W31CCR1VE3C21ABAF60QF/symbols/index.symdb"},
		},
	}
	Open(context.Background(), b, testBlockMeta1)
	return
}

go test --run=Test_Reader_Open_v3_leak -v
=== RUN   Test_Reader_Open_v3_leak
    block_reader_leak_test.go:37: found unexpected goroutines:
        [Goroutine 9 in state select, with go.opencensus.io/stats/view.(*worker).start on top of the stack:
        go.opencensus.io/stats/view.(*worker).start(0x14000a06a80)
        	/Users/liaol/go/pkg/mod/go.opencensus.io@v0.24.0/stats/view/worker.go:292 +0x88
        created by go.opencensus.io/stats/view.init.0 in goroutine 1
        	/Users/liaoliaol/go/pkg/mod/go.opencensus.io@v0.24.0/stats/view/worker.go:34 +0x98
         Goroutine 72 in state chan receive, with github.com/minio/minio-go/v7.(*Client).GetObject.func1 on top of the stack:
        github.com/minio/minio-go/v7.(*Client).GetObject.func1()
        	/Users/liaol/go/pkg/mod/github.com/minio/minio-go/v7@v7.0.72/api-get-object.go:76 +0x134
        created by github.com/minio/minio-go/v7.(*Client).GetObject in goroutine 65
        	/Users/liaol/go/pkg/mod/github.com/minio/minio-go/v7@v7.0.72/api-get-object.go:62 +0x298
        ]
--- FAIL: Test_Reader_Open_v3_leak (1.56s)

[kolesnikovae]

The problem with goleak.VerifyNone in our tests (where we have it), is that we use the fs provider, which does not spawn goroutines – something we could fix