-
-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Use locking to solve data race. #141
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a nit comment 🙏
Co-authored-by: ktr <ktr@syfm.me>
@@ -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() |
There was a problem hiding this comment.
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?
Lines 26 to 28 in 81ff9cb
var defaultOption = opt{ | |
promptString: "> ", | |
} |
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
Codecov Report
@@ Coverage Diff @@
## master #141 +/- ##
==========================================
- Coverage 85.73% 85.67% -0.06%
==========================================
Files 5 5
Lines 764 775 +11
==========================================
+ Hits 655 664 +9
- Misses 95 97 +2
Partials 14 14 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks for your contribution 🎉
thanks for your time! it is ready to rock in my next side project now :) |
Now the following does not show data race anymore.