fix concurrent map write issue by init all regexp when startup #21
Conversation
|
Hi @gorhill. I know that you're not working on this anymore, but I would love to see this fixed one way or another as I'm using your lib and my tests are complaining about that. I didn't inspect the issue too deep when I saw that someone did it already, but if you don't have much time I would love to review it for you. Cheers, |
|
BTW, @aiquestion, you say in #20 that double-checked locks in golang aren't safe. I'm not aware of this so I would love to see an example, and anyway your PR didn't include a double-checked lock. |
|
@elwinar as golang doc : https://golang.org/ref/mem. but actually my previous p-r seems to have another problem: if a goroutine is setting the value, while another goroutine tries to read and check the value, a panic will still occurs. golang map doesn't not allow read and write at the same time. |
|
What I meant was that you only locked once, so it couldn't be a
double-checked lock. Hence the warning in the post you linked: without
the read lock, the go memory model doesn't guarantee you that you're
thread-safe.
Examples are always better. This (from your PR) is not a double-checked
lock:
```
func makeLayoutRegexp(layout, value string) *regexp.Regexp {
layout = strings.Replace(layout, `%value%`, value, -1)
re := layoutRegexp[layout]
if re == nil {
// double check locking to fix issue:
#19
layoutRegexpLock.Lock()
defer layoutRegexpLock.Unlock()
re = layoutRegexp[layout]
if re == nil {
re = regexp.MustCompile(layout)
layoutRegexp[layout] = re
}
}
return re
}
```
This is one:
```
func makeLayoutRegexp(layout, value string) *regexp.Regexp {
layout = strings.Replace(layout, `%value%`, value, -1)
layoutRegexpLock.RLock()
re := layoutRegexp[layout]
if re == nil {
layoutRegexpLock.RUnlock()
// double check locking to fix issue:
#19
layoutRegexpLock.Lock()
defer layoutRegexpLock.Unlock()
re = layoutRegexp[layout]
if re == nil {
re = regexp.MustCompile(layout)
layoutRegexp[layout] = re
}
} else {
layoutRegexpLock.RUnlock()
}
return re
}
```
|
|
ah. you are right. |
this p-r is to fix #19
please help to review it when you get a chance. thx~
i put layoutRegexpMap to fieldDescriptor, and use fieldDescriptor's pointer.
ut passed.
run benchmark, no obvious performance lose.
BenchMark Before:
BenchmarkParse-8 100000 16959 ns/op 4553 B/op 65 allocs/op
BenchmarkNext-8 300000 4873 ns/op 453 B/op 19 allocs/op
After:
BenchmarkParse-8 100000 17210 ns/op 4553 B/op 65 allocs/op
BenchmarkNext-8 300000 4890 ns/op 453 B/op 19 allocs/op