From d17bfad94035eb9bbf0cb9719268b5dcf7dc7276 Mon Sep 17 00:00:00 2001 From: Giteabot Date: Mon, 20 May 2024 13:49:24 +0800 Subject: [PATCH] Fix data-race during testing (#30999) (#31024) Backport #30999 by wxiaoguang Fix #30992 Co-authored-by: wxiaoguang --- models/unit/unit.go | 26 ++++++++++++++++++++------ models/unit/unit_test.go | 24 ++++++++++++------------ tests/integration/org_project_test.go | 6 ++++-- 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/models/unit/unit.go b/models/unit/unit.go index a78a2f1e47..74efa4caf0 100644 --- a/models/unit/unit.go +++ b/models/unit/unit.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "strings" + "sync/atomic" "code.gitea.io/gitea/models/perm" "code.gitea.io/gitea/modules/container" @@ -106,10 +107,23 @@ var ( TypeExternalTracker, } - // DisabledRepoUnits contains the units that have been globally disabled - DisabledRepoUnits = []Type{} + disabledRepoUnitsAtomic atomic.Pointer[[]Type] // the units that have been globally disabled ) +// DisabledRepoUnitsGet returns the globally disabled units, it is a quick patch to fix data-race during testing. +// Because the queue worker might read when a test is mocking the value. FIXME: refactor to a clear solution later. +func DisabledRepoUnitsGet() []Type { + v := disabledRepoUnitsAtomic.Load() + if v == nil { + return nil + } + return *v +} + +func DisabledRepoUnitsSet(v []Type) { + disabledRepoUnitsAtomic.Store(&v) +} + // Get valid set of default repository units from settings func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { units := defaultUnits @@ -127,7 +141,7 @@ func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { } // Remove disabled units - for _, disabledUnit := range DisabledRepoUnits { + for _, disabledUnit := range DisabledRepoUnitsGet() { for i, unit := range units { if unit == disabledUnit { units = append(units[:i], units[i+1:]...) @@ -140,11 +154,11 @@ func validateDefaultRepoUnits(defaultUnits, settingDefaultUnits []Type) []Type { // LoadUnitConfig load units from settings func LoadUnitConfig() error { - var invalidKeys []string - DisabledRepoUnits, invalidKeys = FindUnitTypes(setting.Repository.DisabledRepoUnits...) + disabledRepoUnits, invalidKeys := FindUnitTypes(setting.Repository.DisabledRepoUnits...) if len(invalidKeys) > 0 { log.Warn("Invalid keys in disabled repo units: %s", strings.Join(invalidKeys, ", ")) } + DisabledRepoUnitsSet(disabledRepoUnits) setDefaultRepoUnits, invalidKeys := FindUnitTypes(setting.Repository.DefaultRepoUnits...) if len(invalidKeys) > 0 { @@ -167,7 +181,7 @@ func LoadUnitConfig() error { // UnitGlobalDisabled checks if unit type is global disabled func (u Type) UnitGlobalDisabled() bool { - for _, ud := range DisabledRepoUnits { + for _, ud := range DisabledRepoUnitsGet() { if u == ud { return true } diff --git a/models/unit/unit_test.go b/models/unit/unit_test.go index d80d8b118d..7bf6326145 100644 --- a/models/unit/unit_test.go +++ b/models/unit/unit_test.go @@ -14,10 +14,10 @@ import ( func TestLoadUnitConfig(t *testing.T) { t.Run("regular", func(t *testing.T) { defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnits = disabledRepoUnits + DisabledRepoUnitsSet(disabledRepoUnits) DefaultRepoUnits = defaultRepoUnits DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { setting.Repository.DisabledRepoUnits = disabledRepoUnits setting.Repository.DefaultRepoUnits = defaultRepoUnits @@ -28,16 +28,16 @@ func TestLoadUnitConfig(t *testing.T) { setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls"} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases"} assert.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) + assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) t.Run("invalid", func(t *testing.T) { defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnits = disabledRepoUnits + DisabledRepoUnitsSet(disabledRepoUnits) DefaultRepoUnits = defaultRepoUnits DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { setting.Repository.DisabledRepoUnits = disabledRepoUnits setting.Repository.DefaultRepoUnits = defaultRepoUnits @@ -48,16 +48,16 @@ func TestLoadUnitConfig(t *testing.T) { setting.Repository.DefaultRepoUnits = []string{"repo.code", "invalid.2", "repo.releases", "repo.issues", "repo.pulls"} setting.Repository.DefaultForkRepoUnits = []string{"invalid.3", "repo.releases"} assert.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) + assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) t.Run("duplicate", func(t *testing.T) { defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnits = disabledRepoUnits + DisabledRepoUnitsSet(disabledRepoUnits) DefaultRepoUnits = defaultRepoUnits DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { setting.Repository.DisabledRepoUnits = disabledRepoUnits setting.Repository.DefaultRepoUnits = defaultRepoUnits @@ -68,16 +68,16 @@ func TestLoadUnitConfig(t *testing.T) { setting.Repository.DefaultRepoUnits = []string{"repo.code", "repo.releases", "repo.issues", "repo.pulls", "repo.code"} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"} assert.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) + assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) assert.Equal(t, []Type{TypeCode, TypeReleases, TypePullRequests}, DefaultRepoUnits) assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) t.Run("empty_default", func(t *testing.T) { defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []Type) { - DisabledRepoUnits = disabledRepoUnits + DisabledRepoUnitsSet(disabledRepoUnits) DefaultRepoUnits = defaultRepoUnits DefaultForkRepoUnits = defaultForkRepoUnits - }(DisabledRepoUnits, DefaultRepoUnits, DefaultForkRepoUnits) + }(DisabledRepoUnitsGet(), DefaultRepoUnits, DefaultForkRepoUnits) defer func(disabledRepoUnits, defaultRepoUnits, defaultForkRepoUnits []string) { setting.Repository.DisabledRepoUnits = disabledRepoUnits setting.Repository.DefaultRepoUnits = defaultRepoUnits @@ -88,7 +88,7 @@ func TestLoadUnitConfig(t *testing.T) { setting.Repository.DefaultRepoUnits = []string{} setting.Repository.DefaultForkRepoUnits = []string{"repo.releases", "repo.releases"} assert.NoError(t, LoadUnitConfig()) - assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnits) + assert.Equal(t, []Type{TypeIssues}, DisabledRepoUnitsGet()) assert.ElementsMatch(t, []Type{TypeCode, TypePullRequests, TypeReleases, TypeWiki, TypePackages, TypeProjects, TypeActions}, DefaultRepoUnits) assert.Equal(t, []Type{TypeReleases}, DefaultForkRepoUnits) }) diff --git a/tests/integration/org_project_test.go b/tests/integration/org_project_test.go index ca39cf5130..31d10f16ff 100644 --- a/tests/integration/org_project_test.go +++ b/tests/integration/org_project_test.go @@ -9,13 +9,15 @@ import ( "testing" unit_model "code.gitea.io/gitea/models/unit" - "code.gitea.io/gitea/modules/test" "code.gitea.io/gitea/tests" ) func TestOrgProjectAccess(t *testing.T) { defer tests.PrepareTestEnv(t)() - defer test.MockVariableValue(&unit_model.DisabledRepoUnits, append(slices.Clone(unit_model.DisabledRepoUnits), unit_model.TypeProjects))() + + disabledRepoUnits := unit_model.DisabledRepoUnitsGet() + unit_model.DisabledRepoUnitsSet(append(slices.Clone(disabledRepoUnits), unit_model.TypeProjects)) + defer unit_model.DisabledRepoUnitsSet(disabledRepoUnits) // repo project, 404 req := NewRequest(t, "GET", "/user2/repo1/projects")