git-codereview: avoid race in loadGerritOrigin
Use a mutex in loadGerritOrigin to avoid race when called in parallel
by "git coderevew pending". Add a new initialized field so that the
code knows when auth has been initialized. Adjust tests accordingly.
The test is simply "go test -race".
Fixes golang/go#43670
Change-Id: Ifb060fca6ed463f1d11a2959d03fca5e14e238c6
Reviewed-on: https://go-review.googlesource.com/c/review/+/287012
Trust: Ian Lance Taylor <iant@golang.org>
Run-TryBot: Ian Lance Taylor <iant@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
diff --git a/git-codereview/api.go b/git-codereview/api.go
index c7126f9..afd421f 100644
--- a/git-codereview/api.go
+++ b/git-codereview/api.go
@@ -17,10 +17,13 @@
"runtime"
"sort"
"strings"
+ "sync"
)
// auth holds cached data about authentication to Gerrit.
var auth struct {
+ initialized bool
+
host string // "go.googlesource.com"
url string // "https://go-review.googlesource.com"
project string // "go", "tools", "crypto", etc
@@ -34,12 +37,21 @@
password string
}
+// loadGerritOriginMutex is used to control access when initializing auth
+// in loadGerritOrigin, which can be called in parallel by "pending".
+// We use a mutex rather than a sync.Once because the tests clear auth.
+var loadGerritOriginMutex sync.Mutex
+
// loadGerritOrigin loads the Gerrit host name from the origin remote.
+// This sets auth.{initialized,host,url,project}.
// If the origin remote does not appear to be a Gerrit server
// (is missing, is GitHub, is not https, has too many path elements),
// loadGerritOrigin dies.
func loadGerritOrigin() {
- if auth.host != "" {
+ loadGerritOriginMutex.Lock()
+ defer loadGerritOriginMutex.Unlock()
+
+ if auth.initialized {
return
}
@@ -52,6 +64,8 @@
if err != nil {
dief("failed to load Gerrit origin: %v", err)
}
+
+ auth.initialized = true
}
// loadGerritOriginInternal does the work of loadGerritOrigin, just extracted out
@@ -279,7 +293,7 @@
}
// fullChangeID returns the unambigous Gerrit change ID for the commit c on branch b.
-// The retruned ID has the form project~originbranch~Ihexhexhexhexhex.
+// The returned ID has the form project~originbranch~Ihexhexhexhexhex.
// See https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#change-id for details.
func fullChangeID(b *Branch, c *Commit) string {
loadGerritOrigin()
diff --git a/git-codereview/api_test.go b/git-codereview/api_test.go
index 28ccce9..8494e90 100644
--- a/git-codereview/api_test.go
+++ b/git-codereview/api_test.go
@@ -94,6 +94,7 @@
for i, tt := range authTests {
t.Logf("#%d", i)
+ auth.initialized = false
auth.user = ""
auth.password = ""
auth.cookieName = ""
@@ -175,6 +176,7 @@
}
for _, item := range list {
+ auth.initialized = false
auth.host = ""
auth.url = ""
auth.project = ""
diff --git a/git-codereview/mail_test.go b/git-codereview/mail_test.go
index 93b89f4..72fea82 100644
--- a/git-codereview/mail_test.go
+++ b/git-codereview/mail_test.go
@@ -17,9 +17,11 @@
h := CurrentBranch().Pending()[0].ShortHash
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
@@ -59,9 +61,11 @@
func TestDoNotMailTempFiles(t *testing.T) {
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
@@ -95,9 +99,11 @@
gt.work(t)
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
@@ -182,9 +188,11 @@
defer gt.done()
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
@@ -241,9 +249,11 @@
h := CurrentBranch().Pending()[0].ShortHash
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
@@ -265,9 +275,11 @@
h := CurrentBranch().Pending()[0].ShortHash
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
@@ -290,9 +302,11 @@
defer gt.done()
// fake auth information to avoid Gerrit error
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index 6899213..35e51ac 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -476,10 +476,12 @@
func testPendingArgs(t *testing.T, args []string, want string) {
setHelper(t)
// fake auth information to avoid Gerrit error
- if auth.host == "" {
+ if !auth.initialized {
+ auth.initialized = true
auth.host = "gerrit.fake"
auth.user = "not-a-user"
defer func() {
+ auth.initialized = false
auth.host = ""
auth.user = ""
}()
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index 775b244..fd419a2 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -437,6 +437,7 @@
t.Fatalf("starting fake gerrit: %v", err)
}
+ auth.initialized = true
auth.host = l.Addr().String()
auth.url = "http://" + auth.host
auth.project = "proj"
@@ -450,6 +451,7 @@
func (s *gerritServer) done() {
s.l.Close()
+ auth.initialized = false
auth.host = ""
auth.url = ""
auth.project = ""
