gorhill / cronexpr

Cron expression parser in Go language (golang)
683 stars 168 forks source link

fix concurrent map write issue by init all regexp when startup #21

Open aiquestion opened 7 years ago

aiquestion commented 7 years ago

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

elwinar commented 7 years ago

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 commented 7 years ago

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.

aiquestion commented 7 years ago

@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.

elwinar commented 7 years ago

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
}
aiquestion commented 7 years ago

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