go/buildutil: use chan (not func) in the ForEachPackage API
The callbacks are intentionally concurrent, making this function very
easy to misuse (most clients so far have got it wrong, even my own).
Using a channel in the API makes the concurrency obvious, the
correct usage easy, and the client control flow simpler.
Change-Id: Ied38c3ed5c98b40eb1b322a984ed9dc092ac0918
Reviewed-on: https://go-review.googlesource.com/3250
Reviewed-by: Sameer Ajmani <sameer@golang.org>
diff --git a/go/buildutil/allpackages.go b/go/buildutil/allpackages.go
index c95db42..0f909ee 100644
--- a/go/buildutil/allpackages.go
+++ b/go/buildutil/allpackages.go
@@ -30,11 +30,8 @@
//
func AllPackages(ctxt *build.Context) []string {
var list []string
- var mu sync.Mutex
ForEachPackage(ctxt, func(pkg string, _ error) {
- mu.Lock()
list = append(list, pkg)
- mu.Unlock()
})
sort.Strings(list)
return list
@@ -47,27 +44,42 @@
// If the package directory exists but could not be read, the second
// argument to the found function provides the error.
//
-// The found function and the build.Context file system interface
-// accessors must be concurrency safe.
+// All I/O is done via the build.Context file system interface,
+// which must be concurrency-safe.
//
func ForEachPackage(ctxt *build.Context, found func(importPath string, err error)) {
// We use a counting semaphore to limit
// the number of parallel calls to ReadDir.
sema := make(chan bool, 20)
+ ch := make(chan item)
+
var wg sync.WaitGroup
for _, root := range ctxt.SrcDirs() {
root := root
wg.Add(1)
go func() {
- allPackages(ctxt, sema, root, found)
+ allPackages(ctxt, sema, root, ch)
wg.Done()
}()
}
- wg.Wait()
+ go func() {
+ wg.Wait()
+ close(ch)
+ }()
+
+ // All calls to found occur in the caller's goroutine.
+ for i := range ch {
+ found(i.importPath, i.err)
+ }
}
-func allPackages(ctxt *build.Context, sema chan bool, root string, found func(string, error)) {
+type item struct {
+ importPath string
+ err error // (optional)
+}
+
+func allPackages(ctxt *build.Context, sema chan bool, root string, ch chan<- item) {
root = filepath.Clean(root) + string(os.PathSeparator)
var wg sync.WaitGroup
@@ -92,7 +104,7 @@
files, err := ReadDir(ctxt, dir)
<-sema
if pkg != "" || err != nil {
- found(pkg, err)
+ ch <- item{pkg, err}
}
for _, fi := range files {
fi := fi
diff --git a/refactor/rename/mvpkg.go b/refactor/rename/mvpkg.go
index 4d922f7..bf8edd6 100644
--- a/refactor/rename/mvpkg.go
+++ b/refactor/rename/mvpkg.go
@@ -26,7 +26,6 @@
"runtime"
"strconv"
"strings"
- "sync"
"text/template"
"golang.org/x/tools/go/buildutil"
@@ -133,13 +132,12 @@
// subpackages returns the set of packages in the given srcDir whose
// import paths start with dir.
func subpackages(ctxt *build.Context, srcDir string, dir string) map[string]bool {
- var mu sync.Mutex
subs := map[string]bool{dir: true}
// Find all packages under srcDir whose import paths start with dir.
buildutil.ForEachPackage(ctxt, func(pkg string, err error) {
if err != nil {
- log.Fatalf("unexpected error in ForEackPackage: %v", err)
+ log.Fatalf("unexpected error in ForEachPackage: %v", err)
}
if !strings.HasPrefix(pkg, path.Join(dir, "")) {
@@ -157,9 +155,7 @@
return
}
- mu.Lock()
subs[pkg] = true
- mu.Unlock()
})
return subs
diff --git a/refactor/rename/mvpkg_test.go b/refactor/rename/mvpkg_test.go
index b9f29d6..3c915b4 100644
--- a/refactor/rename/mvpkg_test.go
+++ b/refactor/rename/mvpkg_test.go
@@ -15,7 +15,6 @@
"path/filepath"
"regexp"
"strings"
- "sync"
"testing"
"golang.org/x/tools/go/buildutil"
@@ -212,7 +211,6 @@
for _, test := range tests {
ctxt := test.ctxt
- var mu sync.Mutex
got := make(map[string]string)
// Populate got with starting file set. rewriteFile and moveDirectory
// will mutate got to produce resulting file set.
@@ -225,19 +223,17 @@
return
}
f, err := ctxt.OpenFile(path)
- defer f.Close()
if err != nil {
t.Errorf("unexpected error opening file: %s", err)
return
}
bytes, err := ioutil.ReadAll(f)
+ f.Close()
if err != nil {
t.Errorf("unexpected error reading file: %s", err)
return
}
- mu.Lock()
got[path] = string(bytes)
- defer mu.Unlock()
})
rewriteFile = func(fset *token.FileSet, f *ast.File, orig string) error {
var out bytes.Buffer
