Skip to content

Commit

Permalink
Merge pull request #21 from utilitywarehouse/as-race-fix
Browse files Browse the repository at this point in the history
fix recursive locking and add test to detect deadlocks
  • Loading branch information
asiyani authored Aug 23, 2024
2 parents 36ce679 + d373894 commit 2ba17af
Show file tree
Hide file tree
Showing 7 changed files with 180 additions and 41 deletions.
24 changes: 13 additions & 11 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -4,25 +4,27 @@ on:
push:
pull_request:


permissions:
contents: read

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/...
19 changes: 11 additions & 8 deletions go.mod
Original file line number Diff line number Diff line change
@@ -1,20 +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/prometheus/client_model v0.5.0 // indirect
github.com/prometheus/common v0.48.0 // indirect
github.com/prometheus/procfs v0.12.0 // indirect
golang.org/x/sys v0.17.0 // indirect
google.golang.org/protobuf v1.33.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.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
)
36 changes: 22 additions & 14 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,28 +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/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/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.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=
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=
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.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=
Expand Down
10 changes: 10 additions & 0 deletions pkg/lock/lock_deadlock.go
Original file line number Diff line number Diff line change
@@ -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
}
9 changes: 9 additions & 0 deletions pkg/lock/lock_sync.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
//go:build !deadlock_test

package lock

import "sync"

type RWMutex struct {
sync.RWMutex
}
16 changes: 8 additions & 8 deletions pkg/mirror/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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
Expand All @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
107 changes: 107 additions & 0 deletions pkg/mirror/z_e2e_race_test.go
Original file line number Diff line number Diff line change
@@ -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()
})

}

0 comments on commit 2ba17af

Please sign in to comment.