From 06850cd78b1258a08063241e9958c96050000d6f Mon Sep 17 00:00:00 2001 From: John Cai Date: Fri, 16 Aug 2019 15:38:55 -0700 Subject: [PATCH] protect storages config field with mutex --- internal/cache/walker_test.go | 6 +++--- internal/config/config.go | 14 ++++++++++++++ internal/helper/repo_test.go | 4 ++-- internal/praefect/replicator_test.go | 6 +++--- internal/service/namespace/namespace_test.go | 6 +++--- internal/service/repository/repository_test.go | 4 ++-- internal/service/server/info_test.go | 4 ++-- internal/service/storage/listdirectories_test.go | 4 ++-- internal/service/storage/testhelper_test.go | 2 +- 9 files changed, 32 insertions(+), 18 deletions(-) diff --git a/internal/cache/walker_test.go b/internal/cache/walker_test.go index b3d3044d602..ea4b2e64462 100644 --- a/internal/cache/walker_test.go +++ b/internal/cache/walker_test.go @@ -100,17 +100,17 @@ func setupDiskCacheWalker(t testing.TB) func() { require.NoError(t, err) oldStorages := config.Config.Storages - config.Config.Storages = []config.Storage{ + config.ModifyStorages([]config.Storage{ { Name: t.Name(), Path: tmpPath, }, - } + }) satisfyConfigValidation(tmpPath) cleanup := func() { - config.Config.Storages = oldStorages + config.ModifyStorages(oldStorages) require.NoError(t, os.RemoveAll(tmpPath)) } diff --git a/internal/config/config.go b/internal/config/config.go index fb711adda2c..03d302649c9 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -7,6 +7,7 @@ import ( "os/exec" "path/filepath" "strings" + "sync" "time" "github.com/BurntSushi/toml" @@ -257,6 +258,9 @@ func SetGitPath() error { // StoragePath looks up the base path for storageName. The second boolean // return value indicates if anything was found. func StoragePath(storageName string) (string, bool) { + storageMutex.RLock() + defer storageMutex.RUnlock() + for _, storage := range Config.Storages { if storage.Name == storageName { return storage.Path, true @@ -276,3 +280,13 @@ func validateBinDir() error { Config.BinDir, err = filepath.Abs(Config.BinDir) return err } + +var storageMutex sync.RWMutex + +// ModifyStorages is used in tests to modify the storages in the config in a thread safe way +func ModifyStorages(storages []Storage) { + storageMutex.Lock() + defer storageMutex.Unlock() + + Config.Storages = storages +} diff --git a/internal/helper/repo_test.go b/internal/helper/repo_test.go index c6e9c921b74..4caf340519d 100644 --- a/internal/helper/repo_test.go +++ b/internal/helper/repo_test.go @@ -14,7 +14,7 @@ import ( func TestGetRepoPath(t *testing.T) { defer func(oldStorages []config.Storage) { - config.Config.Storages = oldStorages + config.ModifyStorages(oldStorages) }(config.Config.Storages) testRepo := testhelper.TestRepository() @@ -120,7 +120,7 @@ func TestGetRepoPath(t *testing.T) { for _, tc := range testCases { t.Run(tc.desc, func(t *testing.T) { - config.Config.Storages = tc.storages + config.ModifyStorages(tc.storages) path, err := GetRepoPath(tc.repo) if tc.err != codes.OK { diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go index b669e687592..27f344f82fc 100644 --- a/internal/praefect/replicator_test.go +++ b/internal/praefect/replicator_test.go @@ -148,10 +148,10 @@ func TestReplicate(t *testing.T) { oldStorages := gitaly_config.Config.Storages defer func() { - gitaly_config.Config.Storages = oldStorages + gitaly_config.ModifyStorages(oldStorages) }() - gitaly_config.Config.Storages = append(gitaly_config.Config.Storages, gitaly_config.Storage{ + gitaly_config.ModifyStorages(append(gitaly_config.Config.Storages, gitaly_config.Storage{ Name: backupStorageName, Path: backupDir, }, @@ -159,7 +159,7 @@ func TestReplicate(t *testing.T) { Name: "default", Path: testhelper.GitlabTestStoragePath(), }, - ) + )) ctx, cancel := testhelper.Context() defer cancel() diff --git a/internal/service/namespace/namespace_test.go b/internal/service/namespace/namespace_test.go index f21dc0174a9..968fbb27f6e 100644 --- a/internal/service/namespace/namespace_test.go +++ b/internal/service/namespace/namespace_test.go @@ -23,11 +23,11 @@ func testMain(m *testing.M) int { defer os.Remove(storageOtherDir) oldStorages := config.Config.Storages - config.Config.Storages = []config.Storage{ + config.ModifyStorages([]config.Storage{ {Name: "default", Path: testhelper.GitlabTestStoragePath()}, {Name: "other", Path: storageOtherDir}, - } - defer func() { config.Config.Storages = oldStorages }() + }) + defer func() { config.ModifyStorages(oldStorages) }() return m.Run() } diff --git a/internal/service/repository/repository_test.go b/internal/service/repository/repository_test.go index ea6e91763dc..fa7bfa21915 100644 --- a/internal/service/repository/repository_test.go +++ b/internal/service/repository/repository_test.go @@ -34,9 +34,9 @@ func TestRepositoryExists(t *testing.T) { } defer func(oldStorages []config.Storage) { - config.Config.Storages = oldStorages + config.ModifyStorages(oldStorages) }(config.Config.Storages) - config.Config.Storages = testStorages + config.ModifyStorages(testStorages) queries := []struct { desc string diff --git a/internal/service/server/info_test.go b/internal/service/server/info_test.go index d1b287285cd..37ba0de1366 100644 --- a/internal/service/server/info_test.go +++ b/internal/service/server/info_test.go @@ -34,9 +34,9 @@ func TestGitalyServerInfo(t *testing.T) { {Name: "broken", Path: "/does/not/exist"}, } defer func(oldStorages []config.Storage) { - config.Config.Storages = oldStorages + config.ModifyStorages(oldStorages) }(config.Config.Storages) - config.Config.Storages = testStorages + config.ModifyStorages(testStorages) tempDir, err := ioutil.TempDir("", "gitaly-bin") require.NoError(t, err) diff --git a/internal/service/storage/listdirectories_test.go b/internal/service/storage/listdirectories_test.go index 115d1744422..0e03143ad2f 100644 --- a/internal/service/storage/listdirectories_test.go +++ b/internal/service/storage/listdirectories_test.go @@ -26,9 +26,9 @@ func TestListDirectories(t *testing.T) { testStorages := []config.Storage{{Name: "default", Path: testDir}} defer func(oldStorages []config.Storage) { - config.Config.Storages = oldStorages + config.ModifyStorages(oldStorages) }(config.Config.Storages) - config.Config.Storages = testStorages + config.ModifyStorages(testStorages) repoPaths := []string{"foo", "bar", "bar/baz", "bar/baz/foo/buz"} for _, p := range repoPaths { diff --git a/internal/service/storage/testhelper_test.go b/internal/service/storage/testhelper_test.go index 63da71a6564..335d4188202 100644 --- a/internal/service/storage/testhelper_test.go +++ b/internal/service/storage/testhelper_test.go @@ -36,7 +36,7 @@ func configureTestStorage() { testStorage = config.Storage{Name: "storage-will-be-deleted", Path: storagePath} - config.Config.Storages = []config.Storage{testStorage} + config.ModifyStorages([]config.Storage{testStorage}) } func runStorageServer(t *testing.T) (*grpc.Server, string) { -- GitLab