From c973a8ce163690f23b4758a6728ec355d61784b5 Mon Sep 17 00:00:00 2001 From: wunderbarb Date: Sun, 5 Oct 2025 10:48:26 -0700 Subject: [PATCH 1/3] Added no leak test --- extensions/s3/constructor_s3.go | 49 ++++++++++++++++++---------- extensions/s3/constructor_s3_test.go | 2 +- extensions/s3/go.mod | 6 ++-- extensions/s3/go.sum | 4 +-- extensions/s3/init_test.go | 16 +++++++++ 5 files changed, 54 insertions(+), 23 deletions(-) diff --git a/extensions/s3/constructor_s3.go b/extensions/s3/constructor_s3.go index fb5aa0d..e1c58c8 100644 --- a/extensions/s3/constructor_s3.go +++ b/extensions/s3/constructor_s3.go @@ -4,11 +4,18 @@ package s3 import ( "context" "fmt" + "sync" + "sync/atomic" "github.com/Digital-Shane/treeview" "github.com/Digital-Shane/treeview/extensions/s3/internal/s3" ) +type safeThreadNode struct { + *treeview.Node[treeview.FileInfo] + sync.RWMutex +} + // NewTreeFromS3 creates a new tree structure based on files fetched from an S3 path, using configurable options. // Returns a pointer to a Tree structure or an error if an issue occurs during tree creation. // @@ -38,27 +45,30 @@ func buildFileSystemTreeForS3(ctx context.Context, path string, profile string, if err != nil { return nil, pathError(treeview.ErrPathResolution, path, err) } - total := 1 - rootNode := treeview.NewFileSystemNode(path, info) - cfg.HandleExpansion(rootNode) + total := int64(1) + + rootNode := safeThreadNode{Node: treeview.NewFileSystemNode(path, info), RWMutex: sync.RWMutex{}} + cfg.HandleExpansion(rootNode.Node) if info.IsDir() { - if err := scanDirS3(ctx, rootNode, 0, false, cfg, &total); err != nil { + if err := scanDirS3(ctx, &rootNode, 0, cfg, &total); err != nil { return nil, err } } - return []*treeview.Node[treeview.FileInfo]{rootNode}, nil + return []*treeview.Node[treeview.FileInfo]{rootNode.Node}, nil } // scanDirS3 scans a bucket or key and its subdirectories, creating Node[treeview.FileInfo] for each entry. // It returns an error if the traversal cap is exceeded or if there is an error. -func scanDirS3(ctx context.Context, parent *treeview.Node[treeview.FileInfo], depth int, followSymlinks bool, - cfg *treeview.MasterConfig[treeview.FileInfo], count *int) error { +func scanDirS3(ctx context.Context, parent *safeThreadNode, depth int, cfg *treeview.MasterConfig[treeview.FileInfo], count *int64) error { if cfg.HasDepthLimitBeenReached(depth) { return nil } - entries, err := s3.ReadDir(ctx, parent.Data().Path) + parent.RLock() + p := parent.Data().Path + parent.RUnlock() + entries, err := s3.ReadDir(ctx, p) if err != nil { - return pathError(treeview.ErrDirectoryScan, parent.Data().Path, err) + return pathError(treeview.ErrDirectoryScan, p, err) } children := make([]*treeview.Node[treeview.FileInfo], 0, len(entries)) // preallocation of the capacity only. for _, entry := range entries { @@ -66,7 +76,7 @@ func scanDirS3(ctx context.Context, parent *treeview.Node[treeview.FileInfo], de if err := ctx.Err(); err != nil { return err } - childPath := s3.Join(parent.Data().Path, entry.Name()) // entry.Name is the full key. + childPath := s3.Join(p, entry.Name()) // entry.Name is the full key. info, err := entry.Info() if err != nil { return pathError(treeview.ErrFileSystem, childPath, err) @@ -77,22 +87,27 @@ func scanDirS3(ctx context.Context, parent *treeview.Node[treeview.FileInfo], de }) { continue // Item was filtered out } - childNode := treeview.NewFileSystemNode(childPath, info) - cfg.HandleExpansion(childNode) - *count++ - cfg.ReportProgress(*count, childNode) - if cfg.HasTraversalCapBeenReached(*count) { + childNode := safeThreadNode{Node: treeview.NewFileSystemNode(childPath, info), RWMutex: sync.RWMutex{}} + cfg.HandleExpansion(childNode.Node) + atomic.AddInt64(count, 1) + val := int(atomic.LoadInt64(count)) + cfg.ReportProgress(val, childNode.Node) + if cfg.HasTraversalCapBeenReached(val) { return pathError(treeview.ErrTraversalLimit, childPath, nil) } if info.IsDir() { - if err := scanDirS3(ctx, childNode, depth+1, followSymlinks, cfg, count); err != nil { + if err := scanDirS3(ctx, &childNode, depth+1, cfg, count); err != nil { return err } } - children = append(children, childNode) + childNode.RLock() + children = append(children, childNode.Node) + childNode.RUnlock() } if len(children) > 0 { + parent.Lock() parent.SetChildren(children) + parent.Unlock() } return nil } diff --git a/extensions/s3/constructor_s3_test.go b/extensions/s3/constructor_s3_test.go index 37f757e..bcc4238 100644 --- a/extensions/s3/constructor_s3_test.go +++ b/extensions/s3/constructor_s3_test.go @@ -10,6 +10,7 @@ import ( ) func TestNewTreeFromS3(t *testing.T) { + noLeakButPersistentHTTP(t) tests := []struct { path string @@ -57,6 +58,5 @@ func TestNewTreeFromS3(t *testing.T) { len(tr.Nodes()[0].Children()), ii+1) } } - } } diff --git a/extensions/s3/go.mod b/extensions/s3/go.mod index 603ab86..fce612a 100644 --- a/extensions/s3/go.mod +++ b/extensions/s3/go.mod @@ -2,9 +2,8 @@ module github.com/Digital-Shane/treeview/extensions/s3 go 1.25.0 -replace ( - github.com/Digital-Shane/treeview => ../.. -) +replace github.com/Digital-Shane/treeview => ../.. + require ( github.com/Digital-Shane/treeview v1.8.2 github.com/aws/aws-sdk-go-v2 v1.38.3 @@ -15,6 +14,7 @@ require ( github.com/google/go-cmp v0.7.0 github.com/pkg/errors v0.9.1 github.com/pterm/pterm v0.12.81 + github.com/ysmood/gotrace v0.6.0 ) require ( diff --git a/extensions/s3/go.sum b/extensions/s3/go.sum index fa18901..6080858 100644 --- a/extensions/s3/go.sum +++ b/extensions/s3/go.sum @@ -6,8 +6,6 @@ atomicgo.dev/keyboard v0.2.9 h1:tOsIid3nlPLZ3lwgG8KZMp/SFmr7P0ssEN5JUsm78K8= atomicgo.dev/keyboard v0.2.9/go.mod h1:BC4w9g00XkxH/f1HXhW2sXmJFOCWbKn9xrOunSFtExQ= atomicgo.dev/schedule v0.1.0 h1:nTthAbhZS5YZmgYbb2+DH8uQIZcTlIrd4eYr3UQxEjs= atomicgo.dev/schedule v0.1.0/go.mod h1:xeUa3oAkiuHYh8bKiQBRojqAMq3PXXbJujjb0hw8pEU= -github.com/Digital-Shane/treeview v1.8.1 h1:/Z6Sfs7nnQw2rJHTt/xoVo5ICSMAa1Ij2XnHq1+UOqc= -github.com/Digital-Shane/treeview v1.8.1/go.mod h1:ayLS+EA0aOK9GToyltxW3NTnL8Hag5HQOst6/qV934c= github.com/MarvinJWendt/testza v0.1.0/go.mod h1:7AxNvlfeHP7Z/hDQ5JtE3OKYT3XFUeLCDE2DQninSqs= github.com/MarvinJWendt/testza v0.2.1/go.mod h1:God7bhG8n6uQxwdScay+gjm9/LnO4D3kkcZX4hv9Rp8= github.com/MarvinJWendt/testza v0.2.8/go.mod h1:nwIcjmr0Zz+Rcwfh3/4UhBp7ePKVhuBExvZqnKYWlII= @@ -140,6 +138,8 @@ github.com/stretchr/testify v1.10.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf github.com/xo/terminfo v0.0.0-20210125001918-ca9a967f8778/go.mod h1:2MuV+tbUrU1zIOPMxZ5EncGwgmMJsa+9ucAQZXxsObs= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e h1:JVG44RsyaB9T2KIHavMF/ppJZNG9ZpyihvCd0w101no= github.com/xo/terminfo v0.0.0-20220910002029-abceb7e1c41e/go.mod h1:RbqR21r5mrJuqunuUZ/Dhy/avygyECGrLceyNeo4LiM= +github.com/ysmood/gotrace v0.6.0 h1:SyI1d4jclswLhg7SWTL6os3L1WOKeNn/ZtzVQF8QmdY= +github.com/ysmood/gotrace v0.6.0/go.mod h1:TzhIG7nHDry5//eYZDYcTzuJLYQIkykJzCRIo4/dzQM= github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= diff --git a/extensions/s3/init_test.go b/extensions/s3/init_test.go index 3668b83..9eb0462 100644 --- a/extensions/s3/init_test.go +++ b/extensions/s3/init_test.go @@ -2,8 +2,11 @@ package s3 import ( "path" + "strings" "testing" + "github.com/ysmood/gotrace" + "github.com/Digital-Shane/treeview/extensions/s3/internal/localstack" ) @@ -46,3 +49,16 @@ func isPanic(err error) { panic(err) } } + +// noLeakButPersistentHTTP verifies whether there were no new running go routines after the current test excepted +// the ones that HTTP may create for a persistent connection +func noLeakButPersistentHTTP(t *testing.T) { + ign := gotrace.CombineIgnores( + gotrace.IgnoreCurrent(), + func(t *gotrace.Trace) bool { + return strings.Contains(t.Raw, "net/http.(*persistConn).writeLoop") + }, + gotrace.IgnoreFuncs("internal/poll.runtime_pollWait"), + ) + gotrace.CheckLeak(t, 0, ign) +} From e4d994daa8246545bac6393ca07aa6961235fdec Mon Sep 17 00:00:00 2001 From: wunderbarb Date: Sun, 5 Oct 2025 15:35:45 -0700 Subject: [PATCH 2/3] Cleaning golint issues --- extensions/s3/.golangci.yml | 70 +++++++++++++++++++ extensions/s3/constructor_s3.go | 3 +- .../s3/internal/localstack/localstack_test.go | 2 +- extensions/s3/internal/s3/info_test.go | 2 +- extensions/s3/internal/s3/ops_test.go | 2 +- 5 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 extensions/s3/.golangci.yml diff --git a/extensions/s3/.golangci.yml b/extensions/s3/.golangci.yml new file mode 100644 index 0000000..f52c55e --- /dev/null +++ b/extensions/s3/.golangci.yml @@ -0,0 +1,70 @@ +version: "2" +run: + go: "1.24" + tests: true +linters: + default: none + enable: + - bodyclose + - dogsled + - dupl + - err113 + - errcheck + - exhaustive + - funlen + - goconst + - gocritic + - gocyclo + - goprintffuncname + - gosec + - govet + - ineffassign + - lll + - misspell + - mnd + - nakedret + - noctx + - nolintlint + - revive + - rowserrcheck + - staticcheck + - unconvert + - unparam + - unused + settings: + errcheck: + check-type-assertions: false + check-blank: false + gocognit: + min-complexity: 15 + gocyclo: + min-complexity: 15 + misspell: + locale: US + unparam: + check-exported: false + exclusions: + generated: lax + presets: + - comments + - common-false-positives + - legacy + - std-error-handling + paths: + - third_party$ + - builtin$ + - examples$ +formatters: + enable: + - gofmt + - goimports + settings: + goimports: + local-prefixes: + - github.com/TechDev-SPE/go + exclusions: + generated: lax + paths: + - third_party$ + - builtin$ + - examples$ diff --git a/extensions/s3/constructor_s3.go b/extensions/s3/constructor_s3.go index e1c58c8..1558d02 100644 --- a/extensions/s3/constructor_s3.go +++ b/extensions/s3/constructor_s3.go @@ -59,7 +59,8 @@ func buildFileSystemTreeForS3(ctx context.Context, path string, profile string, // scanDirS3 scans a bucket or key and its subdirectories, creating Node[treeview.FileInfo] for each entry. // It returns an error if the traversal cap is exceeded or if there is an error. -func scanDirS3(ctx context.Context, parent *safeThreadNode, depth int, cfg *treeview.MasterConfig[treeview.FileInfo], count *int64) error { +func scanDirS3(ctx context.Context, parent *safeThreadNode, depth int, cfg *treeview.MasterConfig[treeview.FileInfo], + count *int64) error { if cfg.HasDepthLimitBeenReached(depth) { return nil } diff --git a/extensions/s3/internal/localstack/localstack_test.go b/extensions/s3/internal/localstack/localstack_test.go index a11b144..efcea87 100644 --- a/extensions/s3/internal/localstack/localstack_test.go +++ b/extensions/s3/internal/localstack/localstack_test.go @@ -113,7 +113,7 @@ func randomSmallCapsID() string { choiceSize := len(choice) for i := 0; i < size; i++ { // generates the characters - s := rand.IntN(choiceSize) + s := rand.IntN(choiceSize) // #nosec G404 do not need true randomness. buffer = append(buffer, choice[s]) } return string(buffer) diff --git a/extensions/s3/internal/s3/info_test.go b/extensions/s3/internal/s3/info_test.go index 84f4618..309ed85 100644 --- a/extensions/s3/internal/s3/info_test.go +++ b/extensions/s3/internal/s3/info_test.go @@ -54,7 +54,7 @@ func Test_ReadDir(t *testing.T) { } } -func Test_DirEntry_is_interface(t *testing.T) { +func Test_DirEntry_is_interface(_ *testing.T) { var _ fs.DirEntry = (*DirEntry)(nil) } diff --git a/extensions/s3/internal/s3/ops_test.go b/extensions/s3/internal/s3/ops_test.go index b16e38a..72f9c15 100644 --- a/extensions/s3/internal/s3/ops_test.go +++ b/extensions/s3/internal/s3/ops_test.go @@ -64,7 +64,7 @@ func randomID() string { choiceSize := len(choice) for i := 0; i < size; i++ { // generates the characters - s := rand.IntN(choiceSize) + s := rand.IntN(choiceSize) // #nosec G404 do not need true randomness. buffer = append(buffer, choice[s]) } return string(buffer) From b48a0ba56adc0c3a0a3be0db102247e922942df5 Mon Sep 17 00:00:00 2001 From: wunderbarb Date: Sun, 5 Oct 2025 15:39:55 -0700 Subject: [PATCH 3/3] Preparing the PR --- extensions/s3/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/extensions/s3/CHANGELOG.md b/extensions/s3/CHANGELOG.md index 778f97d..3935143 100644 --- a/extensions/s3/CHANGELOG.md +++ b/extensions/s3/CHANGELOG.md @@ -3,6 +3,9 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [v0.1.1] - 2025-10-05 +### Changed +- S3 constructor uses concurrency to scan the bucket. Thus, the constructor is faster than in v0.1.0. ## [v0.1.0] - 2025-09-19 ### Added