qbit/go
1
0
Fork 0
mirror of https://github.com/golang/go synced 2024-11-18 23:05:06 -07:00
Code Releases Activity

go.tools/importer: crude fix for race condition.

Browse Source 
Revision 8f2c714c6d97 made the 'imports' map per-typechecker,
not per package, breaking an assumption of doImport0.  This
API needs a rethink for a number of reasons, some of which are
noted in this CL.

R=gri
CC=golang-dev
https://golang.org/cl/14606044
This commit is contained in:
Alan Donovan 2013-10-11 15:18:21 -04:00
parent e4256a40f4
commit 7e4be2f6bc
1 changed files with 38 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

40
importer/importer.go
View File

@ -149,8 +149,40 @@ func (imp *Importer) addPackage(info *PackageInfo) {
// the types.Config.Error callback (the first of which is also saved
// in the package's PackageInfo).
//
// Idempotent and thread-safe, but assumes that no two concurrent
// calls will provide the same 'imports' map.
// Idempotent and thread-safe.
//
//
// TODO(gri): The imports map (an alias for TypeChecker.Packages) must
// not be concurrently accessed. Today, the only (non-test) accesses
// of this map are in the client-supplied implementation of the
// importer function, i.e. the function below. But this is a fragile
// API because if go/types ever starts to access this map, it and its
// clients will have to agree to use the same mutex.
// Two better ideas:
//
// (1) require that the importer functions be stateful and have this
// map be part of that internal state.
// Pro: good encapsulation.
// Con: we can't have different importers collaborate, e.g.
// we can't use a source importer for some packages and
// GcImport for others since they'd each have a distinct map.
//
// (2) have there be a single map in go/types.Config, but expose
// lookup and update behind an interface and pass that interface
// to the importer implementations.
// Pro: sharing of the map among importers.
//
// This is idempotent but still doesn't address the issue of
// atomicity: when loading packages concurrently, we want to avoid
// the benign but suboptimal situation of two goroutines both
// importing "fmt", finding it not present, doing all the work, and
// double-updating the map.
// The interface to the map needs to express the idea that when a
// caller requests an import from the map and finds it not present,
// then it (and no other goroutine) becomes responsible for loading
// the package and updating the map; other goroutines should block
// until then. That's exactly what doImport0 below does; I think
// some of that logic should migrate into go/types.check.resolveFiles.
//
func (imp *Importer) doImport(imports map[string]*types.Package, path string) (*types.Package, error) {
// Package unsafe is handled specially, and has no PackageInfo.
@ -173,12 +205,16 @@ func (imp *Importer) doImport(imports map[string]*types.Package, path string) (*
// contains direct or transitive dependencies, it probably
// shouldn't be exposed. This package can make its own
// arrangements to implement PackageInfo.Imports().
importsMu.Lock()
imports[path] = info.Pkg
importsMu.Unlock()
}
return info.Pkg, nil
}
var importsMu sync.Mutex // hack; see comment at doImport
// Like doImport, but returns a PackageInfo.
// Precondition: path != "unsafe".
func (imp *Importer) doImport0(imports map[string]*types.Package, path string) (*PackageInfo, error) {