From 247b3b3a5a6c21089ba311cc732ad394e8bd0278 Mon Sep 17 00:00:00 2001 From: Ashok Siyani Date: Fri, 23 Aug 2024 12:30:38 +0100 Subject: [PATCH 1/3] add race test to detect deadlocks --- .github/workflows/test.yaml | 24 ++++---- go.mod | 2 + go.sum | 4 ++ pkg/lock/lock_deadlock.go | 10 ++++ pkg/lock/lock_sync.go | 9 +++ pkg/mirror/repository.go | 4 +- pkg/mirror/z_e2e_race_test.go | 107 ++++++++++++++++++++++++++++++++++ 7 files changed, 147 insertions(+), 13 deletions(-) create mode 100644 pkg/lock/lock_deadlock.go create mode 100644 pkg/lock/lock_sync.go create mode 100644 pkg/mirror/z_e2e_race_test.go diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index e314548..c62c270 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -4,7 +4,6 @@ on: push: pull_request: - permissions: contents: read @@ -12,17 +11,20 @@ jobs: pkg-test: strategy: matrix: - go-version: [1.21.x, 1.22.x] + go-version: [1.21.x, 1.22.x, 1.23.x] os: [ubuntu-latest] runs-on: ${{ matrix.os }} steps: - - name: Checkout code - uses: actions/checkout@v4 + - name: Checkout code + uses: actions/checkout@v4 + + - name: Install Go + uses: actions/setup-go@v5 + with: + go-version: ${{ matrix.go-version }} + + - name: Test + run: go test -v -cover ./... - - name: Install Go - uses: actions/setup-go@v5 - with: - go-version: ${{ matrix.go-version }} - - - name: Test - run: go test -v -cover ./... + - name: Test with race + run: go test -v -cover -race -count 1 -timeout 20s --tags deadlock_test -run Test_mirror_detect_race ./pkg/mirror/... diff --git a/go.mod b/go.mod index e78f8a8..9aa313b 100644 --- a/go.mod +++ b/go.mod @@ -12,9 +12,11 @@ require ( github.com/beorn7/perks v1.0.1 // indirect github.com/cespare/xxhash/v2 v2.2.0 // indirect github.com/kr/text v0.2.0 // indirect + github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect github.com/prometheus/client_model v0.5.0 // indirect github.com/prometheus/common v0.48.0 // indirect github.com/prometheus/procfs v0.12.0 // indirect + github.com/sasha-s/go-deadlock v0.3.5 // indirect golang.org/x/sys v0.17.0 // indirect google.golang.org/protobuf v1.33.0 // indirect ) diff --git a/go.sum b/go.sum index cae1299..356d02a 100644 --- a/go.sum +++ b/go.sum @@ -9,6 +9,8 @@ github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw= +github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4= github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE= github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho= github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw= @@ -19,6 +21,8 @@ github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= +github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU= +github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U= golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI= diff --git a/pkg/lock/lock_deadlock.go b/pkg/lock/lock_deadlock.go new file mode 100644 index 0000000..e4138fa --- /dev/null +++ b/pkg/lock/lock_deadlock.go @@ -0,0 +1,10 @@ +//go:build deadlock_test + +package lock + +import "github.com/sasha-s/go-deadlock" + +// this type is used only in test for deadlock detection +type RWMutex struct { + deadlock.RWMutex +} diff --git a/pkg/lock/lock_sync.go b/pkg/lock/lock_sync.go new file mode 100644 index 0000000..84258d3 --- /dev/null +++ b/pkg/lock/lock_sync.go @@ -0,0 +1,9 @@ +//go:build !deadlock_test + +package lock + +import "sync" + +type RWMutex struct { + sync.RWMutex +} diff --git a/pkg/mirror/repository.go b/pkg/mirror/repository.go index 143114e..f800a66 100644 --- a/pkg/mirror/repository.go +++ b/pkg/mirror/repository.go @@ -11,10 +11,10 @@ import ( "regexp" "slices" "strings" - "sync" "time" "github.com/utilitywarehouse/git-mirror/pkg/giturl" + "github.com/utilitywarehouse/git-mirror/pkg/lock" ) const ( @@ -49,6 +49,7 @@ const ( // The implementation borrows heavily from https://github.com/kubernetes/git-sync. // A Repository is safe for concurrent use by multiple goroutines. type Repository struct { + lock lock.RWMutex // repository will be locked during mirror gitURL *giturl.URL // parsed remote git URL remote string // remote repo to mirror root string // absolute path to the root where repo directory createdabsolute path to the root where repo directory created @@ -60,7 +61,6 @@ type Repository struct { envs []string // envs which will be passed to git commands running bool // indicates if repository is running the mirror loop workTreeLinks map[string]*WorkTreeLink // list of worktrees which will be maintained - lock sync.RWMutex // repository will be locked during mirror stop, stopped chan bool // chans to stop mirror loops log *slog.Logger } diff --git a/pkg/mirror/z_e2e_race_test.go b/pkg/mirror/z_e2e_race_test.go new file mode 100644 index 0000000..687304a --- /dev/null +++ b/pkg/mirror/z_e2e_race_test.go @@ -0,0 +1,107 @@ +//go:build deadlock_test + +package mirror + +import ( + "context" + "log" + "os" + "path/filepath" + "sync" + "testing" +) + +func Test_mirror_detect_race(t *testing.T) { + testTmpDir := mustTmpDir(t) + defer os.RemoveAll(testTmpDir) + + ctx, cancel := context.WithCancel(context.TODO()) + defer cancel() + + upstream := filepath.Join(testTmpDir, testUpstreamRepo) + root := filepath.Join(testTmpDir, testRoot) + link1 := "link1" // on testBranchMain branch + link2 := "link2" // on remote HEAD + ref1 := testMainBranch + ref2 := "HEAD" + testName := t.Name() + + t.Log("TEST-1: init upstream") + fileSHA1 := mustInitRepo(t, upstream, "file", testName+"-1") + + repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1) + // add worktree for HEAD + if err := repo.AddWorktreeLink(link2, ref2, ""); err != nil { + t.Fatalf("unable to add worktree error: %v", err) + } + // mirror again for 2nd worktree + if err := repo.Mirror(ctx); err != nil { + t.Fatalf("unable to mirror error: %v", err) + } + + // verify checkout files + assertCommitLog(t, repo, "HEAD", "", fileSHA1, testName+"-1", []string{"file"}) + assertLinkedFile(t, root, link1, "file", testName+"-1") + assertLinkedFile(t, root, link2, "file", testName+"-1") + + // start mirror loop + go repo.StartLoop(ctx) + close(repo.stop) + + t.Log("TEST-2: forward HEAD") + fileSHA2 := mustCommit(t, upstream, "file", testName+"-2") + + t.Run("test-1", func(t *testing.T) { + wg := &sync.WaitGroup{} + // all following assertions will always be true + // this test is about testing deadlocks and detecting race conditions + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if err := repo.Mirror(ctx); err != nil { + log.Fatalf("unable to mirror error: %v", err) + } + + assertLinkedFile(t, root, link1, "file", testName+"-2") + assertLinkedFile(t, root, link2, "file", testName+"-2") + }() + + wg.Add(1) + go func() { + defer wg.Done() + + assertCommitLog(t, repo, "HEAD", "", fileSHA2, testName+"-2", []string{"file"}) + }() + } + + // clone tests + for i := 0; i < 10; i++ { + wg.Add(1) + go func() { + defer wg.Done() + if err := repo.Mirror(ctx); err != nil { + log.Fatalf("unable to mirror error: %v", err) + } + }() + + wg.Add(1) + go func() { + defer wg.Done() + tempClone := mustTmpDir(t) + defer os.RemoveAll(tempClone) + + if cloneSHA, err := repo.Clone(ctx, tempClone, testMainBranch, "", i%2 == 0); err != nil { + t.Fatalf("unexpected error %s", err) + } else { + if cloneSHA != fileSHA2 { + t.Errorf("clone sha mismatch got:%s want:%s", cloneSHA, fileSHA2) + } + assertFile(t, filepath.Join(tempClone, "file"), testName+"-2") + } + }() + } + wg.Wait() + }) + +} From 17c4fa5e67a7f627b9468996bc19555a6ac977cc Mon Sep 17 00:00:00 2001 From: Ashok Siyani Date: Fri, 23 Aug 2024 12:32:10 +0100 Subject: [PATCH 2/3] fixed recursive locking --- pkg/mirror/repository.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/mirror/repository.go b/pkg/mirror/repository.go index f800a66..ca7e470 100644 --- a/pkg/mirror/repository.go +++ b/pkg/mirror/repository.go @@ -188,13 +188,13 @@ func (r *Repository) LogMsg(ctx context.Context, ref, path string) (string, erro // Subject returns commit subject of given commit hash func (r *Repository) Subject(ctx context.Context, hash string) (string, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if err := r.ObjectExists(ctx, hash); err != nil { return "", err } + r.lock.RLock() + defer r.lock.RUnlock() + args := []string{"show", `--no-patch`, `--format='%s'`, hash} msg, err := runGitCommand(ctx, r.log, r.envs, r.dir, args...) if err != nil { @@ -205,13 +205,13 @@ func (r *Repository) Subject(ctx context.Context, hash string) (string, error) { // ChangedFiles returns path of the changed files for given commit hash func (r *Repository) ChangedFiles(ctx context.Context, hash string) ([]string, error) { - r.lock.RLock() - defer r.lock.RUnlock() - if err := r.ObjectExists(ctx, hash); err != nil { return nil, err } + r.lock.RLock() + defer r.lock.RUnlock() + args := []string{"show", `--name-only`, `--pretty=format:`, hash} msg, err := runGitCommand(ctx, r.log, r.envs, r.dir, args...) if err != nil { From d373894f7b471179df7f32ab0e70c434426a1fcd Mon Sep 17 00:00:00 2001 From: Ashok Siyani Date: Fri, 23 Aug 2024 12:50:27 +0100 Subject: [PATCH 3/3] updated go and dependencies --- go.mod | 19 ++++++++++--------- go.sum | 32 ++++++++++++++++++-------------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/go.mod b/go.mod index 9aa313b..10fbd1c 100644 --- a/go.mod +++ b/go.mod @@ -1,22 +1,23 @@ module github.com/utilitywarehouse/git-mirror -go 1.22.0 +go 1.23.0 require ( github.com/google/go-cmp v0.6.0 - github.com/prometheus/client_golang v1.19.1 + github.com/prometheus/client_golang v1.20.1 + github.com/sasha-s/go-deadlock v0.3.5 gopkg.in/yaml.v3 v3.0.1 ) require ( github.com/beorn7/perks v1.0.1 // indirect - github.com/cespare/xxhash/v2 v2.2.0 // indirect + github.com/cespare/xxhash/v2 v2.3.0 // indirect github.com/kr/text v0.2.0 // indirect + github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect - github.com/prometheus/client_model v0.5.0 // indirect - github.com/prometheus/common v0.48.0 // indirect - github.com/prometheus/procfs v0.12.0 // indirect - github.com/sasha-s/go-deadlock v0.3.5 // indirect - golang.org/x/sys v0.17.0 // indirect - google.golang.org/protobuf v1.33.0 // indirect + github.com/prometheus/client_model v0.6.1 // indirect + github.com/prometheus/common v0.55.0 // indirect + github.com/prometheus/procfs v0.15.1 // indirect + golang.org/x/sys v0.24.0 // indirect + google.golang.org/protobuf v1.34.2 // indirect ) diff --git a/go.sum b/go.sum index 356d02a..7061dbb 100644 --- a/go.sum +++ b/go.sum @@ -1,32 +1,36 @@ github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM= github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw= -github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44= -github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= +github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs= +github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs= github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E= github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI= github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= +github.com/klauspost/compress v1.17.9 h1:6KIumPrER1LHsvBVuDa0r5xaG0Es51mhhB9BQB2qeMA= +github.com/klauspost/compress v1.17.9/go.mod h1:Di0epgTjJY877eYKx5yC51cX2A2Vl2ibi7bDH9ttBbw= github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE= github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= +github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA= +github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ= github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw= github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4= -github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE= -github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho= -github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw= -github.com/prometheus/client_model v0.5.0/go.mod h1:dTiFglRmd66nLR9Pv9f0mZi7B7fk5Pm3gvsjB5tr+kI= -github.com/prometheus/common v0.48.0 h1:QO8U2CdOzSn1BBsmXJXduaaW+dY/5QLjfB8svtSzKKE= -github.com/prometheus/common v0.48.0/go.mod h1:0/KsvlIEfPQCQ5I2iNSAWKPZziNCvRs5EC6ILDTlAPc= -github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo= -github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo= +github.com/prometheus/client_golang v1.20.1 h1:IMJXHOD6eARkQpxo8KkhgEVFlBNm+nkrFUyGlIu7Na8= +github.com/prometheus/client_golang v1.20.1/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE= +github.com/prometheus/client_model v0.6.1 h1:ZKSh/rekM+n3CeS952MLRAdFwIKqeY8b62p8ais2e9E= +github.com/prometheus/client_model v0.6.1/go.mod h1:OrxVMOVHjw3lKMa8+x6HeMGkHMQyHDk9E3jmP2AmGiY= +github.com/prometheus/common v0.55.0 h1:KEi6DK7lXW/m7Ig5i47x0vRzuBsHuvJdi5ee6Y3G1dc= +github.com/prometheus/common v0.55.0/go.mod h1:2SECS4xJG1kd8XF9IcM1gMX6510RAEL65zxzNImwdc8= +github.com/prometheus/procfs v0.15.1 h1:YagwOFzUgYfKKHX6Dr+sHT7km/hxC76UB0learggepc= +github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoGhij/e3PBqk= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU= github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U= -golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y= -golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= -google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI= -google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos= +golang.org/x/sys v0.24.0 h1:Twjiwq9dn6R1fQcyiK+wQyHWfaz/BJB+YIpzU/Cv3Xg= +golang.org/x/sys v0.24.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA= +google.golang.org/protobuf v1.34.2 h1:6xV6lTsCfpGD21XK49h7MhtcApnLqkfYgPcdHftf6hg= +google.golang.org/protobuf v1.34.2/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk= gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=