From bdcea2c1b36a391fd43708dcb8bdca48adf3d724 Mon Sep 17 00:00:00 2001
From: Alan Donovan <adonovan@google.com>
Date: Fri, 23 Jan 2015 14:53:23 -0500
Subject: [PATCH] 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>
---
 go/buildutil/allpackages.go   | 30 +++++++++++++++++++++---------
 refactor/rename/mvpkg.go      |  6 +-----
 refactor/rename/mvpkg_test.go |  6 +-----
 3 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/go/buildutil/allpackages.go b/go/buildutil/allpackages.go
index c95db42c35..0f909eeb20 100644
--- a/go/buildutil/allpackages.go
+++ b/go/buildutil/allpackages.go
@@ -30,11 +30,8 @@ import (
 //
 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 @@ func AllPackages(ctxt *build.Context) []string {
 // 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 @@ func allPackages(ctxt *build.Context, sema chan bool, root string, found func(st
 		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 4d922f76fd..bf8edd6f5a 100644
--- a/refactor/rename/mvpkg.go
+++ b/refactor/rename/mvpkg.go
@@ -26,7 +26,6 @@ import (
 	"runtime"
 	"strconv"
 	"strings"
-	"sync"
 	"text/template"
 
 	"golang.org/x/tools/go/buildutil"
@@ -133,13 +132,12 @@ func srcDir(ctxt *build.Context, pkg string) (string, error) {
 // 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 @@ func subpackages(ctxt *build.Context, srcDir string, dir string) map[string]bool
 			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 b9f29d6b6a..3c915b44a2 100644
--- a/refactor/rename/mvpkg_test.go
+++ b/refactor/rename/mvpkg_test.go
@@ -15,7 +15,6 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
-	"sync"
 	"testing"
 
 	"golang.org/x/tools/go/buildutil"
@@ -212,7 +211,6 @@ var _ a.T
 	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 @@ var _ a.T
 				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