From 8255f49be1c3e3fcc0ded9ba08025d34c4389df4 Mon Sep 17 00:00:00 2001 From: JackKCWong Date: Fri, 4 Mar 2022 15:19:20 +0800 Subject: [PATCH] Use locking to solve data race. (#141) * Use locking to solve data race. * add more doc * Update doc on WithHotReload Co-authored-by: ktr * fix tests Co-authored-by: ktr --- .gitignore | 1 + example/go.mod | 2 ++ example/go.sum | 2 -- example/hotreload/main.go | 8 ++++- fuzzyfinder.go | 6 ++++ fuzzyfinder_test.go | 34 +++++++++++++++++++ option.go | 32 +++++++++++++---- .../fixtures/testfind_hotreloadlock.golden | 11 ++++++ 8 files changed, 86 insertions(+), 10 deletions(-) create mode 100644 testdata/fixtures/testfind_hotreloadlock.golden diff --git a/.gitignore b/.gitignore index 31933d5..2a88ce8 100644 --- a/.gitignore +++ b/.gitignore @@ -16,3 +16,4 @@ coverage.txt fuzz.out _tools +.idea diff --git a/example/go.mod b/example/go.mod index bbb302d..1e967a1 100644 --- a/example/go.mod +++ b/example/go.mod @@ -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 diff --git a/example/go.sum b/example/go.sum index 25022d8..9832d25 100644 --- a/example/go.sum +++ b/example/go.sum @@ -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= diff --git a/example/hotreload/main.go b/example/hotreload/main.go index 909c47b..baefd77 100644 --- a/example/hotreload/main.go +++ b/example/hotreload/main.go @@ -5,6 +5,8 @@ import ( "fmt" "log" "os" + "sync" + "time" fuzzyfinder "github.com/ktr0731/go-fuzzyfinder" isatty "github.com/mattn/go-isatty" @@ -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) @@ -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) diff --git a/fuzzyfinder.go b/fuzzyfinder.go index 36c1f45..09283ea 100644 --- a/fuzzyfinder.go +++ b/fuzzyfinder.go @@ -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() @@ -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() rvv := reflect.Indirect(rv) items, matched = makeItems(rvv.Len()) + opt.hotReloadLock.Unlock() go func() { <-inited @@ -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 } } diff --git a/fuzzyfinder_test.go b/fuzzyfinder_test.go index fc0f7ec..a97735a 100644 --- a/fuzzyfinder_test.go +++ b/fuzzyfinder_test.go @@ -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 diff --git a/option.go b/option.go index 25512e2..1943e37 100644 --- a/option.go +++ b/option.go @@ -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 @@ -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. @@ -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 ( diff --git a/testdata/fixtures/testfind_hotreloadlock.golden b/testdata/fixtures/testfind_hotreloadlock.golden new file mode 100644 index 0000000..4a567d3 --- /dev/null +++ b/testdata/fixtures/testfind_hotreloadlock.golden @@ -0,0 +1,11 @@ + ┌────────────────────────────┐ + │ Name: adrenaline!!! │ + │ Artist: TrySail │ + │ │ + │ │ + │ │ + │ │ +> adrenaline!!! │ │ + 1/9 │ │ +> adrena█ └────────────────────────────┘ + \ No newline at end of file