Skip to content

Commit 2ba17af

Browse files
asiyaniasiyani
authored
Merge pull request #21 from utilitywarehouse/as-race-fix
fix recursive locking and add test to detect deadlocks
2 parents 36ce679 + d373894 commit 2ba17af

File tree

7 files changed

+180
-41
lines changed

7 files changed

+180
-41
lines changed

Diff for: ‎.github/workflows/test.yaml

+13-11
Original file line numberDiff line numberDiff line change
@@ -4,25 +4,27 @@ on:
44
push:
55
pull_request:
66

7-
87
permissions:
98
contents: read
109

1110
jobs:
1211
pkg-test:
1312
strategy:
1413
matrix:
15-
go-version: [1.21.x, 1.22.x]
14+
go-version: [1.21.x, 1.22.x, 1.23.x]
1615
os: [ubuntu-latest]
1716
runs-on: ${{ matrix.os }}
1817
steps:
19-
- name: Checkout code
20-
uses: actions/checkout@v4
18+
- name: Checkout code
19+
uses: actions/checkout@v4
20+
21+
- name: Install Go
22+
uses: actions/setup-go@v5
23+
with:
24+
go-version: ${{ matrix.go-version }}
25+
26+
- name: Test
27+
run: go test -v -cover ./...
2128

22-
- name: Install Go
23-
uses: actions/setup-go@v5
24-
with:
25-
go-version: ${{ matrix.go-version }}
26-
27-
- name: Test
28-
run: go test -v -cover ./...
29+
- name: Test with race
30+
run: go test -v -cover -race -count 1 -timeout 20s --tags deadlock_test -run Test_mirror_detect_race ./pkg/mirror/...

Diff for: ‎go.mod

+11-8
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,23 @@
11
module github.com/utilitywarehouse/git-mirror
22

3-
go 1.22.0
3+
go 1.23.0
44

55
require (
66
github.com/google/go-cmp v0.6.0
7-
github.com/prometheus/client_golang v1.19.1
7+
github.com/prometheus/client_golang v1.20.1
8+
github.com/sasha-s/go-deadlock v0.3.5
89
gopkg.in/yaml.v3 v3.0.1
910
)
1011

1112
require (
1213
github.com/beorn7/perks v1.0.1 // indirect
13-
github.com/cespare/xxhash/v2 v2.2.0 // indirect
14+
github.com/cespare/xxhash/v2 v2.3.0 // indirect
1415
github.com/kr/text v0.2.0 // indirect
15-
github.com/prometheus/client_model v0.5.0 // indirect
16-
github.com/prometheus/common v0.48.0 // indirect
17-
github.com/prometheus/procfs v0.12.0 // indirect
18-
golang.org/x/sys v0.17.0 // indirect
19-
google.golang.org/protobuf v1.33.0 // indirect
16+
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect
17+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 // indirect
18+
github.com/prometheus/client_model v0.6.1 // indirect
19+
github.com/prometheus/common v0.55.0 // indirect
20+
github.com/prometheus/procfs v0.15.1 // indirect
21+
golang.org/x/sys v0.24.0 // indirect
22+
google.golang.org/protobuf v1.34.2 // indirect
2023
)

Diff for: ‎go.sum

+22-14
Original file line numberDiff line numberDiff line change
@@ -1,28 +1,36 @@
11
github.com/beorn7/perks v1.0.1 h1:VlbKKnNfV8bJzeqoa4cOKqO6bYr3WgKZxO8Z16+hsOM=
22
github.com/beorn7/perks v1.0.1/go.mod h1:G2ZrVWU2WbWT9wwq4/hrbKbnv/1ERSJQ0ibhJ6rlkpw=
3-
github.com/cespare/xxhash/v2 v2.2.0 h1:DC2CZ1Ep5Y4k3ZQ899DldepgrayRUGE6BBZ/cd9Cj44=
4-
github.com/cespare/xxhash/v2 v2.2.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
3+
github.com/cespare/xxhash/v2 v2.3.0 h1:UL815xU9SqsFlibzuggzjXhog7bL6oX9BbNZnL2UFvs=
4+
github.com/cespare/xxhash/v2 v2.3.0/go.mod h1:VGX0DQ3Q6kWi7AoAeZDth3/j3BFtOZR5XLFGgcrjCOs=
55
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
66
github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
77
github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
8+
github.com/klauspost/compress v1.17.9 h1:6KIumPrER1LHsvBVuDa0r5xaG0Es51mhhB9BQB2qeMA=
9+
github.com/klauspost/compress v1.17.9/go.mod h1:Di0epgTjJY877eYKx5yC51cX2A2Vl2ibi7bDH9ttBbw=
810
github.com/kr/pretty v0.3.1 h1:flRD4NNwYAUpkphVc1HcthR4KEIFJ65n8Mw5qdRn3LE=
911
github.com/kr/pretty v0.3.1/go.mod h1:hoEshYVHaxMs3cyo3Yncou5ZscifuDolrwPKZanG3xk=
1012
github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
1113
github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
12-
github.com/prometheus/client_golang v1.19.1 h1:wZWJDwK+NameRJuPGDhlnFgx8e8HN3XHQeLaYJFJBOE=
13-
github.com/prometheus/client_golang v1.19.1/go.mod h1:mP78NwGzrVks5S2H6ab8+ZZGJLZUq1hoULYBAYBw1Ho=
14-
github.com/prometheus/client_model v0.5.0 h1:VQw1hfvPvk3Uv6Qf29VrPF32JB6rtbgI6cYPYQjL0Qw=
15-
github.com/prometheus/client_model v0.5.0/go.mod h1:dTiFglRmd66nLR9Pv9f0mZi7B7fk5Pm3gvsjB5tr+kI=
16-
github.com/prometheus/common v0.48.0 h1:QO8U2CdOzSn1BBsmXJXduaaW+dY/5QLjfB8svtSzKKE=
17-
github.com/prometheus/common v0.48.0/go.mod h1:0/KsvlIEfPQCQ5I2iNSAWKPZziNCvRs5EC6ILDTlAPc=
18-
github.com/prometheus/procfs v0.12.0 h1:jluTpSng7V9hY0O2R9DzzJHYb2xULk9VTR1V1R/k6Bo=
19-
github.com/prometheus/procfs v0.12.0/go.mod h1:pcuDEFsWDnvcgNzo4EEweacyhjeA9Zk3cnaOZAZEfOo=
14+
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 h1:C3w9PqII01/Oq1c1nUAm88MOHcQC9l5mIlSMApZMrHA=
15+
github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822/go.mod h1:+n7T8mK8HuQTcFwEeznm/DIxMOiR9yIdICNftLE1DvQ=
16+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7 h1:Dx7Ovyv/SFnMFw3fD4oEoeorXc6saIiQ23LrGLth0Gw=
17+
github.com/petermattis/goid v0.0.0-20240813172612-4fcff4a6cae7/go.mod h1:pxMtw7cyUw6B2bRH0ZBANSPg+AoSud1I1iyJHI69jH4=
18+
github.com/prometheus/client_golang v1.20.1 h1:IMJXHOD6eARkQpxo8KkhgEVFlBNm+nkrFUyGlIu7Na8=
19+
github.com/prometheus/client_golang v1.20.1/go.mod h1:PIEt8X02hGcP8JWbeHyeZ53Y/jReSnHgO035n//V5WE=
20+
github.com/prometheus/client_model v0.6.1 h1:ZKSh/rekM+n3CeS952MLRAdFwIKqeY8b62p8ais2e9E=
21+
github.com/prometheus/client_model v0.6.1/go.mod h1:OrxVMOVHjw3lKMa8+x6HeMGkHMQyHDk9E3jmP2AmGiY=
22+
github.com/prometheus/common v0.55.0 h1:KEi6DK7lXW/m7Ig5i47x0vRzuBsHuvJdi5ee6Y3G1dc=
23+
github.com/prometheus/common v0.55.0/go.mod h1:2SECS4xJG1kd8XF9IcM1gMX6510RAEL65zxzNImwdc8=
24+
github.com/prometheus/procfs v0.15.1 h1:YagwOFzUgYfKKHX6Dr+sHT7km/hxC76UB0learggepc=
25+
github.com/prometheus/procfs v0.15.1/go.mod h1:fB45yRUv8NstnjriLhBQLuOUt+WW4BsoGhij/e3PBqk=
2026
github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ=
2127
github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
22-
golang.org/x/sys v0.17.0 h1:25cE3gD+tdBA7lp7QfhuV+rJiE9YXTcS3VG1SqssI/Y=
23-
golang.org/x/sys v0.17.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
24-
google.golang.org/protobuf v1.33.0 h1:uNO2rsAINq/JlFpSdYEKIZ0uKD/R9cpdv0T+yoGwGmI=
25-
google.golang.org/protobuf v1.33.0/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
28+
github.com/sasha-s/go-deadlock v0.3.5 h1:tNCOEEDG6tBqrNDOX35j/7hL5FcFViG6awUGROb2NsU=
29+
github.com/sasha-s/go-deadlock v0.3.5/go.mod h1:bugP6EGbdGYObIlx7pUZtWqlvo8k9H6vCBBsiChJQ5U=
30+
golang.org/x/sys v0.24.0 h1:Twjiwq9dn6R1fQcyiK+wQyHWfaz/BJB+YIpzU/Cv3Xg=
31+
golang.org/x/sys v0.24.0/go.mod h1:/VUhepiaJMQUp4+oa/7Zr1D23ma6VTLIYjOOTFZPUcA=
32+
google.golang.org/protobuf v1.34.2 h1:6xV6lTsCfpGD21XK49h7MhtcApnLqkfYgPcdHftf6hg=
33+
google.golang.org/protobuf v1.34.2/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw=
2634
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
2735
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
2836
gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=

Diff for: ‎pkg/lock/lock_deadlock.go

+10
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
//go:build deadlock_test
2+
3+
package lock
4+
5+
import "github.com/sasha-s/go-deadlock"
6+
7+
// this type is used only in test for deadlock detection
8+
type RWMutex struct {
9+
deadlock.RWMutex
10+
}

Diff for: ‎pkg/lock/lock_sync.go

+9
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
//go:build !deadlock_test
2+
3+
package lock
4+
5+
import "sync"
6+
7+
type RWMutex struct {
8+
sync.RWMutex
9+
}

Diff for: ‎pkg/mirror/repository.go

+8-8
Original file line numberDiff line numberDiff line change
@@ -11,10 +11,10 @@ import (
1111
"regexp"
1212
"slices"
1313
"strings"
14-
"sync"
1514
"time"
1615

1716
"github.com/utilitywarehouse/git-mirror/pkg/giturl"
17+
"github.com/utilitywarehouse/git-mirror/pkg/lock"
1818
)
1919

2020
const (
@@ -49,6 +49,7 @@ const (
4949
// The implementation borrows heavily from https://github.com/kubernetes/git-sync.
5050
// A Repository is safe for concurrent use by multiple goroutines.
5151
type Repository struct {
52+
lock lock.RWMutex // repository will be locked during mirror
5253
gitURL *giturl.URL // parsed remote git URL
5354
remote string // remote repo to mirror
5455
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 {
6061
envs []string // envs which will be passed to git commands
6162
running bool // indicates if repository is running the mirror loop
6263
workTreeLinks map[string]*WorkTreeLink // list of worktrees which will be maintained
63-
lock sync.RWMutex // repository will be locked during mirror
6464
stop, stopped chan bool // chans to stop mirror loops
6565
log *slog.Logger
6666
}
@@ -188,13 +188,13 @@ func (r *Repository) LogMsg(ctx context.Context, ref, path string) (string, erro
188188

189189
// Subject returns commit subject of given commit hash
190190
func (r *Repository) Subject(ctx context.Context, hash string) (string, error) {
191-
r.lock.RLock()
192-
defer r.lock.RUnlock()
193-
194191
if err := r.ObjectExists(ctx, hash); err != nil {
195192
return "", err
196193
}
197194

195+
r.lock.RLock()
196+
defer r.lock.RUnlock()
197+
198198
args := []string{"show", `--no-patch`, `--format='%s'`, hash}
199199
msg, err := runGitCommand(ctx, r.log, r.envs, r.dir, args...)
200200
if err != nil {
@@ -205,13 +205,13 @@ func (r *Repository) Subject(ctx context.Context, hash string) (string, error) {
205205

206206
// ChangedFiles returns path of the changed files for given commit hash
207207
func (r *Repository) ChangedFiles(ctx context.Context, hash string) ([]string, error) {
208-
r.lock.RLock()
209-
defer r.lock.RUnlock()
210-
211208
if err := r.ObjectExists(ctx, hash); err != nil {
212209
return nil, err
213210
}
214211

212+
r.lock.RLock()
213+
defer r.lock.RUnlock()
214+
215215
args := []string{"show", `--name-only`, `--pretty=format:`, hash}
216216
msg, err := runGitCommand(ctx, r.log, r.envs, r.dir, args...)
217217
if err != nil {

Diff for: ‎pkg/mirror/z_e2e_race_test.go

+107
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,107 @@
1+
//go:build deadlock_test
2+
3+
package mirror
4+
5+
import (
6+
"context"
7+
"log"
8+
"os"
9+
"path/filepath"
10+
"sync"
11+
"testing"
12+
)
13+
14+
func Test_mirror_detect_race(t *testing.T) {
15+
testTmpDir := mustTmpDir(t)
16+
defer os.RemoveAll(testTmpDir)
17+
18+
ctx, cancel := context.WithCancel(context.TODO())
19+
defer cancel()
20+
21+
upstream := filepath.Join(testTmpDir, testUpstreamRepo)
22+
root := filepath.Join(testTmpDir, testRoot)
23+
link1 := "link1" // on testBranchMain branch
24+
link2 := "link2" // on remote HEAD
25+
ref1 := testMainBranch
26+
ref2 := "HEAD"
27+
testName := t.Name()
28+
29+
t.Log("TEST-1: init upstream")
30+
fileSHA1 := mustInitRepo(t, upstream, "file", testName+"-1")
31+
32+
repo := mustCreateRepoAndMirror(t, upstream, root, link1, ref1)
33+
// add worktree for HEAD
34+
if err := repo.AddWorktreeLink(link2, ref2, ""); err != nil {
35+
t.Fatalf("unable to add worktree error: %v", err)
36+
}
37+
// mirror again for 2nd worktree
38+
if err := repo.Mirror(ctx); err != nil {
39+
t.Fatalf("unable to mirror error: %v", err)
40+
}
41+
42+
// verify checkout files
43+
assertCommitLog(t, repo, "HEAD", "", fileSHA1, testName+"-1", []string{"file"})
44+
assertLinkedFile(t, root, link1, "file", testName+"-1")
45+
assertLinkedFile(t, root, link2, "file", testName+"-1")
46+
47+
// start mirror loop
48+
go repo.StartLoop(ctx)
49+
close(repo.stop)
50+
51+
t.Log("TEST-2: forward HEAD")
52+
fileSHA2 := mustCommit(t, upstream, "file", testName+"-2")
53+
54+
t.Run("test-1", func(t *testing.T) {
55+
wg := &sync.WaitGroup{}
56+
// all following assertions will always be true
57+
// this test is about testing deadlocks and detecting race conditions
58+
for i := 0; i < 100; i++ {
59+
wg.Add(1)
60+
go func() {
61+
defer wg.Done()
62+
if err := repo.Mirror(ctx); err != nil {
63+
log.Fatalf("unable to mirror error: %v", err)
64+
}
65+
66+
assertLinkedFile(t, root, link1, "file", testName+"-2")
67+
assertLinkedFile(t, root, link2, "file", testName+"-2")
68+
}()
69+
70+
wg.Add(1)
71+
go func() {
72+
defer wg.Done()
73+
74+
assertCommitLog(t, repo, "HEAD", "", fileSHA2, testName+"-2", []string{"file"})
75+
}()
76+
}
77+
78+
// clone tests
79+
for i := 0; i < 10; i++ {
80+
wg.Add(1)
81+
go func() {
82+
defer wg.Done()
83+
if err := repo.Mirror(ctx); err != nil {
84+
log.Fatalf("unable to mirror error: %v", err)
85+
}
86+
}()
87+
88+
wg.Add(1)
89+
go func() {
90+
defer wg.Done()
91+
tempClone := mustTmpDir(t)
92+
defer os.RemoveAll(tempClone)
93+
94+
if cloneSHA, err := repo.Clone(ctx, tempClone, testMainBranch, "", i%2 == 0); err != nil {
95+
t.Fatalf("unexpected error %s", err)
96+
} else {
97+
if cloneSHA != fileSHA2 {
98+
t.Errorf("clone sha mismatch got:%s want:%s", cloneSHA, fileSHA2)
99+
}
100+
assertFile(t, filepath.Join(tempClone, "file"), testName+"-2")
101+
}
102+
}()
103+
}
104+
wg.Wait()
105+
})
106+
107+
}

0 commit comments

Comments
 (0)