cronexpr icon indicating copy to clipboard operation
cronexpr copied to clipboard

fix concurrent map write issue by init all regexp when startup

Open aiquestion opened this issue 9 years ago • 5 comments

this p-r is to fix https://github.com/gorhill/cronexpr/issues/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

aiquestion avatar Nov 29 '16 02:11 aiquestion

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, R;

elwinar avatar Jan 06 '17 14:01 elwinar

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 avatar Jan 06 '17 14:01 elwinar

@elwinar as golang doc : https://golang.org/ref/mem. and this blog: http://blog.launchdarkly.com/golang-pearl-thread-safe-writes-and-double-checked-locking-in-go/ i need to use sync/atomic to load and set checked value in double checked locking. So i choose the suggested way once.Do(). :-) this function is a correct implementation of double checked locking :-)

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.

aiquestion avatar Jan 07 '17 04:01 aiquestion

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: 
https://github.com/gorhill/cronexpr/issues/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: 
https://github.com/gorhill/cronexpr/issues/19
		layoutRegexpLock.Lock()
		defer layoutRegexpLock.Unlock()
		re = layoutRegexp[layout]
		if re == nil {
			re = regexp.MustCompile(layout)
			layoutRegexp[layout] = re
		}
	} else {
		layoutRegexpLock.RUnlock()
	}
	return re
}

elwinar avatar Jan 07 '17 10:01 elwinar

ah. you are right. I think i wrote a java-pattern double checked locking in my previous p-r which is wrong in golang.

aiquestion avatar Jan 10 '17 02:01 aiquestion