WIP: Fix data race (Repository.Units)

.drone.yml

@@ -396,6 +396,7 @@ steps:
     environment:
       GOPROXY: off
       TAGS: bindata sqlite sqlite_unlock_notify
+      RACE_ENABLED: "0"
 
   - name: gpg-sign
     pull: always
@@ -498,6 +499,7 @@ steps:
     environment:
       GOPROXY: off
       TAGS: bindata sqlite sqlite_unlock_notify
+      RACE_ENABLED: "0"
 
   - name: gpg-sign
     pull: always

Makefile

@@ -27,6 +27,11 @@ EXTRA_GOFLAGS ?=
 
 MAKE_VERSION := $(shell make -v | head -n 1)
 
+ifneq ($(RACE_ENABLED),"0")
+GOFLAGS += -race
+GOTESTFLAGS += -race
+endif
+
 ifneq ($(DRONE_TAG),)
 	VERSION ?= $(subst v,,$(DRONE_TAG))
 	GITEA_VERSION ?= $(VERSION)
@@ -166,7 +171,7 @@ fmt-check:
 
 .PHONY: test
 test:
-	GO111MODULE=on $(GO) test -mod=vendor -tags='sqlite sqlite_unlock_notify' $(PACKAGES)
+	GO111MODULE=on $(GO) test $(GOTESTFLAGS) -mod=vendor -tags='sqlite sqlite_unlock_notify' $(PACKAGES)
 
 .PHONY: coverage
 coverage:
@@ -177,7 +182,7 @@ coverage:
 
 .PHONY: unit-test-coverage
 unit-test-coverage:
-	$(GO) test -tags='sqlite sqlite_unlock_notify' -cover -coverprofile coverage.out $(PACKAGES) && echo "\n==>\033[32m Ok\033[m\n" || exit 1
+	$(GO) test $(GOTESTFLAGS) -tags='sqlite sqlite_unlock_notify' -cover -coverprofile coverage.out $(PACKAGES) && echo "\n==>\033[32m Ok\033[m\n" || exit 1
 
 .PHONY: vendor
 vendor:
@@ -281,21 +286,21 @@ integration-test-coverage: integrations.cover.test generate-ini
 	GITEA_ROOT=${CURDIR} GITEA_CONF=integrations/mysql.ini ./integrations.cover.test -test.coverprofile=integration.coverage.out
 
 integrations.test: $(SOURCES)
-	GO111MODULE=on $(GO) test -mod=vendor -c code.gitea.io/gitea/integrations -o integrations.test
+	GO111MODULE=on $(GO) test $(GOTESTFLAGS) -mod=vendor -c code.gitea.io/gitea/integrations -o integrations.test
 
 integrations.sqlite.test: $(SOURCES)
-	GO111MODULE=on $(GO) test -mod=vendor -c code.gitea.io/gitea/integrations -o integrations.sqlite.test -tags 'sqlite sqlite_unlock_notify'
+	GO111MODULE=on $(GO) test $(GOTESTFLAGS) -mod=vendor -c code.gitea.io/gitea/integrations -o integrations.sqlite.test -tags 'sqlite sqlite_unlock_notify'
 
 integrations.cover.test: $(SOURCES)
-	GO111MODULE=on $(GO) test -mod=vendor -c code.gitea.io/gitea/integrations -coverpkg $(shell echo $(PACKAGES) | tr ' ' ',') -o integrations.cover.test
+	GO111MODULE=on $(GO) test $(GOTESTFLAGS) -mod=vendor -c code.gitea.io/gitea/integrations -coverpkg $(shell echo $(PACKAGES) | tr ' ' ',') -o integrations.cover.test
 
 .PHONY: migrations.test
 migrations.test: $(SOURCES)
-	$(GO) test -c code.gitea.io/gitea/integrations/migration-test -o migrations.test
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/integrations/migration-test -o migrations.test
 
 .PHONY: migrations.sqlite.test
 migrations.sqlite.test: $(SOURCES)
-	$(GO) test -c code.gitea.io/gitea/integrations/migration-test -o migrations.sqlite.test -tags 'sqlite sqlite_unlock_notify'
+	$(GO) test $(GOTESTFLAGS) -c code.gitea.io/gitea/integrations/migration-test -o migrations.sqlite.test -tags 'sqlite sqlite_unlock_notify'
 
 .PHONY: check
 check: test

models/issue_milestone.go

@@ -7,7 +7,6 @@ package models
 import (
 	"fmt"
 
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/modules/timeutil"
@@ -394,46 +393,6 @@ func ChangeMilestoneAssign(issue *Issue, doer *User, oldMilestoneID int64) (err
 	if err = sess.Commit(); err != nil {
 		return fmt.Errorf("Commit: %v", err)
 	}
-
-	var hookAction api.HookIssueAction
-	if issue.MilestoneID > 0 {
-		hookAction = api.HookIssueMilestoned
-	} else {
-		hookAction = api.HookIssueDemilestoned
-	}
-
-	if err = issue.LoadAttributes(); err != nil {
-		return err
-	}
-
-	mode, _ := AccessLevel(doer, issue.Repo)
-	if issue.IsPull {
-		err = issue.PullRequest.LoadIssue()
-		if err != nil {
-			log.Error("LoadIssue: %v", err)
-			return
-		}
-		err = PrepareWebhooks(issue.Repo, HookEventPullRequest, &api.PullRequestPayload{
-			Action:      hookAction,
-			Index:       issue.Index,
-			PullRequest: issue.PullRequest.APIFormat(),
-			Repository:  issue.Repo.APIFormat(mode),
-			Sender:      doer.APIFormat(),
-		})
-	} else {
-		err = PrepareWebhooks(issue.Repo, HookEventIssues, &api.IssuePayload{
-			Action:     hookAction,
-			Index:      issue.Index,
-			Issue:      issue.APIFormat(),
-			Repository: issue.Repo.APIFormat(mode),
-			Sender:     doer.APIFormat(),
-		})
-	}
-	if err != nil {
-		log.Error("PrepareWebhooks [is_pull: %v]: %v", issue.IsPull, err)
-	} else {
-		go HookQueue.Add(issue.RepoID)
-	}
 	return nil
 }
 

models/repo.go

@@ -161,7 +161,7 @@ type Repository struct {
 	*Mirror  `xorm:"-"`
 
 	ExternalMetas map[string]string `xorm:"-"`
-	Units         []*RepoUnit       `xorm:"-"`
+	Units         *RepoUnitList     `xorm:"-"`
 
 	IsFork                          bool               `xorm:"INDEX NOT NULL DEFAULT false"`
 	ForkID                          int64              `xorm:"INDEX"`
@@ -340,8 +340,12 @@ func (repo *Repository) getUnits(e Engine) (err error) {
 		return nil
 	}
 
-	repo.Units, err = getUnitsByRepoID(e, repo.ID)
-	return err
+	units, err := getUnitsByRepoID(e, repo.ID)
+	if err != nil {
+		return err
+	}
+	repo.Units = NewRepoUnitList(units)
+	return nil
 }
 
 // CheckUnitUser check whether user could visit the unit of this repository
@@ -370,12 +374,15 @@ func (repo *Repository) UnitEnabled(tp UnitType) bool {
 	if err := repo.getUnits(x); err != nil {
 		log.Warn("Error loading repository (ID: %d) units: %s", repo.ID, err.Error())
 	}
-	for _, unit := range repo.Units {
+	result := false
+	repo.Units.Range(func(i int, unit *RepoUnit) bool {
 		if unit.Type == tp {
-			return true
+			result = true
+			return false
 		}
-	}
-	return false
+		return true
+	})
+	return result
 }
 
 // ErrUnitTypeNotExist represents a "UnitTypeNotExist" kind of error.
@@ -436,10 +443,17 @@ func (repo *Repository) getUnit(e Engine, tp UnitType) (*RepoUnit, error) {
 	if err := repo.getUnits(e); err != nil {
 		return nil, err
 	}
-	for _, unit := range repo.Units {
+
+	var result *RepoUnit
+	repo.Units.Range(func(i int, unit *RepoUnit) bool {
 		if unit.Type == tp {
-			return unit, nil
+			result = unit
+			return false
 		}
+		return true
+	})
+	if result != nil {
+		return result, nil
 	}
 	return nil, ErrUnitTypeNotExist{tp}
 }

models/repo_permission.go

@@ -13,7 +13,7 @@ import (
 // Permission contains all the permissions related variables to a repository for a user
 type Permission struct {
 	AccessMode AccessMode
-	Units      []*RepoUnit
+	Units      *RepoUnitList
 	UnitsMode  map[UnitType]AccessMode
 }
 
@@ -38,12 +38,15 @@ func (p *Permission) HasAccess() bool {
 // UnitAccessMode returns current user accessmode to the specify unit of the repository
 func (p *Permission) UnitAccessMode(unitType UnitType) AccessMode {
 	if p.UnitsMode == nil {
-		for _, u := range p.Units {
+		result := AccessModeNone
+		p.Units.Range(func(i int, u *RepoUnit) bool {
 			if u.Type == unitType {
-				return p.AccessMode
+				result = p.AccessMode
+				return false
 			}
-		}
-		return AccessModeNone
+			return true
+		})
+		return result
 	}
 	return p.UnitsMode[unitType]
 }
@@ -103,11 +106,11 @@ func (p *Permission) ColorFormat(s fmt.State) {
 	format := "AccessMode: %-v, %d Units, %d UnitsMode(s): [ "
 	args := []interface{}{
 		p.AccessMode,
-		log.NewColoredValueBytes(len(p.Units), &noColor),
+		log.NewColoredValueBytes(p.Units.Len(), &noColor),
 		log.NewColoredValueBytes(len(p.UnitsMode), &noColor),
 	}
 	if s.Flag('+') {
-		for i, unit := range p.Units {
+		p.Units.Range(func(i int, unit *RepoUnit) bool {
 			config := ""
 			if unit.Config != nil {
 				configBytes, err := unit.Config.ToDB()
@@ -123,7 +126,8 @@ func (p *Permission) ColorFormat(s fmt.State) {
 				log.NewColoredIDValue(unit.RepoID),
 				unit.Type,
 				config)
-		}
+			return true
+		})
 		for key, value := range p.UnitsMode {
 			format += "\nUnitMode[%-v]: %-v"
 			args = append(args,
@@ -218,9 +222,10 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss
 
 	// Collaborators on organization
 	if isCollaborator {
-		for _, u := range repo.Units {
+		repo.Units.Range(func(i int, u *RepoUnit) bool {
 			perm.UnitsMode[u.Type] = perm.AccessMode
-		}
+			return true
+		})
 	}
 
 	// get units mode from teams
@@ -238,7 +243,7 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss
 		}
 	}
 
-	for _, u := range repo.Units {
+	repo.Units.Range(func(i int, u *RepoUnit) bool {
 		var found bool
 		for _, team := range teams {
 			if team.unitEnabled(e, u.Type) {
@@ -256,16 +261,18 @@ func getUserRepoPermission(e Engine, repo *Repository, user *User) (perm Permiss
 				perm.UnitsMode[u.Type] = AccessModeRead
 			}
 		}
-	}
+		return true
+	})
 
 	// remove no permission units
-	perm.Units = make([]*RepoUnit, 0, len(repo.Units))
+	perm.Units = NewRepoUnitList(make([]*RepoUnit, 0, repo.Units.Len()))
 	for t := range perm.UnitsMode {
-		for _, u := range repo.Units {
+		repo.Units.Range(func(_ int, u *RepoUnit) bool {
 			if u.Type == t {
-				perm.Units = append(perm.Units, u)
+				perm.Units = AppendRepoUnitList(perm.Units, u)
 			}
-		}
+			return true
+		})
 	}
 
 	return