diff options
author | Alan Donovan <adonovan@google.com> | 2015-01-23 14:53:23 -0500 |
---|---|---|
committer | Alan Donovan <adonovan@google.com> | 2015-03-05 20:14:20 +0000 |
commit | bdcea2c1b36a391fd43708dcb8bdca48adf3d724 (patch) | |
tree | aca97e0a22501dc42b200f5e8f1df304015fd747 | |
parent | 9957739054eda1f0e99582dad0d702da8a3d7d66 (diff) | |
download | tools-bdcea2c1b36a391fd43708dcb8bdca48adf3d724.tar.gz |
go/buildutil: use chan (not func) in the ForEachPackage APIHEADgradle_1.3.0-beta4gradle_1.3.0-beta3mastermain
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>
-rw-r--r-- | go/buildutil/allpackages.go | 30 | ||||
-rw-r--r-- | refactor/rename/mvpkg.go | 6 | ||||
-rw-r--r-- | 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 c95db42..0f909ee 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) + } +} + +type item struct { + importPath string + err error // (optional) } -func allPackages(ctxt *build.Context, sema chan bool, root string, found func(string, error)) { +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 4d922f7..bf8edd6 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 b9f29d6..3c915b4 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 |