From 82515cf89538b3e8a206be74377c01dee420b76d Mon Sep 17 00:00:00 2001 From: Michael Matloob Date: Thu, 2 Aug 2018 15:35:27 -0400 Subject: [PATCH] go/packages: fix race detected by race detector Diamonds in the dependency graph could cause the same package to be processed by two separate gorotines, causing a potential race between checking if the package's Types field is set, and setting the field. adonovan's comment suggested this might be unnecessary. We'll ask him about this once he returns from his vacation. Change-Id: I27511b6e44bb1b6a04ea294206fd8ed4ea60f56f Reviewed-on: https://go-review.googlesource.com/127636 Run-TryBot: Michael Matloob Reviewed-by: Ian Cottrell Reviewed-by: Suzy Mueller TryBot-Result: Gobot Gobot --- go/packages/packages.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/go/packages/packages.go b/go/packages/packages.go index e869af0b589..e1e38f2e13e 100644 --- a/go/packages/packages.go +++ b/go/packages/packages.go @@ -505,6 +505,10 @@ func (ld *loader) loadPackage(lpkg *loaderPackage) { // used to supply alternative file contents. return nil, fmt.Errorf("no metadata for %s", path) } + + ld.exportMu.Lock() + defer ld.exportMu.Unlock() + if ipkg.Types != nil && ipkg.Types.Complete() { return ipkg.Types, nil } @@ -629,8 +633,16 @@ func (ld *loader) loadFromExportData(lpkg *loaderPackage) (*types.Package, error // Not all accesses to Package.Pkg need to be protected by exportMu: // graph ordering ensures that direct dependencies of source // packages are fully loaded before the importer reads their Pkg field. - ld.exportMu.Lock() - defer ld.exportMu.Unlock() + // + // TODO(matloob): Ask adonovan if this is safe. Because of diamonds in the + // dependency graph, it's possible that the same package is loaded in + // two separate goroutines and there's a race in the write of the Package's + // Types here and the read earlier checking if types is set before calling + // this function. + /* + ld.exportMu.Lock() + defer ld.exportMu.Unlock() + */ if tpkg := lpkg.Types; tpkg != nil && tpkg.Complete() { return tpkg, nil // cache hit