Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use locking to solve data race. #141

Merged
merged 4 commits into from
Mar 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,4 @@
coverage.txt
fuzz.out
_tools
.idea
2 changes: 2 additions & 0 deletions example/go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ module github.com/ktr0731/go-fuzzyfinder/example

go 1.17

replace github.com/ktr0731/go-fuzzyfinder => ../

require (
github.com/ktr0731/go-fuzzyfinder v0.5.1
github.com/mattn/go-isatty v0.0.14
Expand Down
2 changes: 0 additions & 2 deletions example/go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@ github.com/google/go-cmp v0.5.6 h1:BKbKCqvP6I+rmFHt06ZmyQtvB8xAkWdhFyr0ZUNZcxQ=
github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
github.com/ktr0731/go-fuzzyfinder v0.5.1 h1:rDcWxmGi6ux4NURekn9iAXpbYBp8Kj4cznrz162S9og=
github.com/ktr0731/go-fuzzyfinder v0.5.1/go.mod h1:gud27uRG2vF+oD58eGhYZj7Pc9enRX0qecwp09w/jno=
github.com/lucasb-eyer/go-colorful v1.0.3 h1:QIbQXiugsb+q10B+MI+7DI1oQLdmnep86tWFlaaUAac=
github.com/lucasb-eyer/go-colorful v1.0.3/go.mod h1:R4dSotOR9KMtayYi1e77YzuveK+i7ruzyGqttikkLy0=
github.com/mattn/go-isatty v0.0.14 h1:yVuAays6BHfxijgZPzw+3Zlu5yQgKGP2/hcQbHb7S9Y=
Expand Down
8 changes: 7 additions & 1 deletion example/hotreload/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"fmt"
"log"
"os"
"sync"
"time"

fuzzyfinder "github.com/ktr0731/go-fuzzyfinder"
isatty "github.com/mattn/go-isatty"
Expand All @@ -19,10 +21,14 @@ func main() {
return
}
var slice []string
var mut sync.RWMutex
go func(slice *[]string) {
s := bufio.NewScanner(os.Stdin)
for s.Scan() {
mut.Lock()
*slice = append(*slice, s.Text())
mut.Unlock()
time.Sleep(50 * time.Millisecond) // to give a feeling of how it looks like in the terminal
}
}(&slice)

Expand All @@ -31,7 +37,7 @@ func main() {
func(i int) string {
return slice[i]
},
fuzzyfinder.WithHotReload(),
fuzzyfinder.WithHotReloadLock(mut.RLocker()),
)
if err != nil {
log.Fatal(err)
Expand Down
6 changes: 6 additions & 0 deletions fuzzyfinder.go
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,10 @@ func (f *finder) initFinder(items []string, matched []matching.Matched, opt opt)

if !isInTesting() {
f.drawTimer = time.AfterFunc(0, func() {
f.stateMu.Lock()
f._draw()
f._drawPreview()
f.stateMu.Unlock()
f.term.Show()
})
f.drawTimer.Stop()
Expand Down Expand Up @@ -615,8 +617,10 @@ func (f *finder) find(slice interface{}, itemFunc func(i int) string, opts []Opt

inited := make(chan struct{})
if opt.hotReload && rv.Kind() == reflect.Ptr {
opt.hotReloadLock.Lock()
Copy link
Owner

@ktr0731 ktr0731 Mar 2, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, it looks like when the caller uses WithHotReload, it will panic because opt.hotReloadLock is nil (see CI result).
Could you add a nop locker as the default option for backward-compatibility?

go-fuzzyfinder/option.go

Lines 26 to 28 in 81ff9cb

var defaultOption = opt{
promptString: "> ",
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ya sure. my mistake. My first ever PR, please bear with me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be passing this time.

go-fuzzyfinder git:(main) go test -race ./...
ok      github.com/ktr0731/go-fuzzyfinder       10.554s
ok      github.com/ktr0731/go-fuzzyfinder/matching      0.638s
ok      github.com/ktr0731/go-fuzzyfinder/scoring       1.046s

rvv := reflect.Indirect(rv)
items, matched = makeItems(rvv.Len())
opt.hotReloadLock.Unlock()

go func() {
<-inited
Expand All @@ -627,11 +631,13 @@ func (f *finder) find(slice interface{}, itemFunc func(i int) string, opts []Opt
case <-ctx.Done():
return
case <-time.After(30 * time.Millisecond):
opt.hotReloadLock.Lock()
curr := rvv.Len()
if prev != curr {
items, matched = makeItems(curr)
f.updateItems(items, matched)
}
opt.hotReloadLock.Unlock()
prev = curr
}
}
Expand Down
34 changes: 34 additions & 0 deletions fuzzyfinder_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,40 @@ func TestFind_hotReload(t *testing.T) {
})
}

func TestFind_hotReloadLock(t *testing.T) {
f, term := fuzzyfinder.NewWithMockedTerminal()
events := append(runes("adrena"), keys(input{tcell.KeyEsc, rune(tcell.KeyEsc), tcell.ModNone})...)
term.SetEventsV2(events...)

var mu sync.RWMutex
assertWithGolden(t, func(t *testing.T) string {
_, err := f.Find(
&tracks,
func(i int) string {
return tracks[i].Name
},
fuzzyfinder.WithPreviewWindow(func(i, width, height int) string {
// Hack, wait until updateItems is called.
time.Sleep(50 * time.Millisecond)
mu.RLock()
defer mu.RUnlock()
if i == -1 {
return "not found"
}
return "Name: " + tracks[i].Name + "\nArtist: " + tracks[i].Artist
}),
fuzzyfinder.WithMode(fuzzyfinder.ModeCaseSensitive),
fuzzyfinder.WithHotReloadLock(mu.RLocker()),
)
if !errors.Is(err, fuzzyfinder.ErrAbort) {
t.Fatalf("Find must return ErrAbort, but got '%s'", err)
}

res := term.GetResult()
return res
})
}

func TestFind_enter(t *testing.T) {
cases := map[string]struct {
events []tcell.Event
Expand Down
32 changes: 25 additions & 7 deletions option.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
package fuzzyfinder

import "sync"

type opt struct {
mode mode
previewFunc func(i, width, height int) string
multi bool
hotReload bool
promptString string
header string
beginAtTop bool
mode mode
previewFunc func(i, width, height int) string
multi bool
hotReload bool
hotReloadLock sync.Locker
promptString string
header string
beginAtTop bool
}

type mode int
Expand All @@ -25,6 +28,7 @@ const (

var defaultOption = opt{
promptString: "> ",
hotReloadLock: &sync.Mutex{}, // this won't resolve the race condition but avoid nil panic
}

// Option represents available fuzzy-finding options.
Expand Down Expand Up @@ -53,12 +57,26 @@ func WithPreviewWindow(f func(i, width, height int) string) Option {

// WithHotReload reloads the passed slice automatically when some entries are appended.
// The caller must pass a pointer of the slice instead of the slice itself.
//
// Deprecated: use WithHotReloadLock instead.
func WithHotReload() Option {
return func(o *opt) {
o.hotReload = true
}
}

// WithHotReloadLock reloads the passed slice automatically when some entries are appended.
// The caller must pass a pointer of the slice instead of the slice itself.
// The caller must pass a RLock which is used to synchronize access to the slice.
// The caller MUST NOT lock in the itemFunc passed to Find / FindMulti because it will be locked by the fuzzyfinder.
// If used together with WithPreviewWindow, the caller MUST use the RLock only in the previewFunc passed to WithPreviewWindow.
func WithHotReloadLock(lock sync.Locker) Option {
return func(o *opt) {
o.hotReload = true
o.hotReloadLock = lock
}
}

type cursorPosition int

const (
Expand Down
11 changes: 11 additions & 0 deletions testdata/fixtures/testfind_hotreloadlock.golden
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
┌────────────────────────────┐
│ Name: adrenaline!!! │
│ Artist: TrySail │
│ │
│ │
│ │
│ │
> adrenaline!!! │ │
1/9 │ │
> adrena█ └────────────────────────────┘