From 54d5b4cb30ff7fbfd136c77570e3266293d040cc Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Thu, 13 Jun 2024 09:47:46 +0300 Subject: [PATCH 1/3] Use sync.OnceValue for lazy regexp initialization Using sync.OnceValue instead of the local implementation brings some benefits in allocations, panic handling and mutex use / concurrent access after initialization. Signed-off-by: Kimmo Lehto --- regexes.go | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/regexes.go b/regexes.go index 7e1dd5a03..f0dc25811 100644 --- a/regexes.go +++ b/regexes.go @@ -80,14 +80,9 @@ const ( ) func lazyRegexCompile(str string) func() *regexp.Regexp { - var regex *regexp.Regexp - var once sync.Once - return func() *regexp.Regexp { - once.Do(func() { - regex = regexp.MustCompile(str) - }) - return regex - } + return sync.OnceValue(func() *regexp.Regexp { + return regexp.MustCompile(str) + }) } var ( From f959b0fb98d62465df2681cc576dcf9bb54956f0 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Thu, 13 Jun 2024 10:09:28 +0300 Subject: [PATCH 2/3] Use sync.OnceValue only on go1.21+ sync.OnceValue was introduced in go1.21. Currently validator's go.mod states minimum go version to be go1.18. This commit moves the lazy regex initialization into its own file lazy.go and provides backwards compatibility using buildtags and the file lazy_compat.go which contains a backported (non-generic) version of sync.OnceValue. Signed-off-by: Kimmo Lehto --- lazy.go | 14 ++++++++++++++ lazy_compat.go | 46 ++++++++++++++++++++++++++++++++++++++++++++++ regexes.go | 11 ----------- 3 files changed, 60 insertions(+), 11 deletions(-) create mode 100644 lazy.go create mode 100644 lazy_compat.go diff --git a/lazy.go b/lazy.go new file mode 100644 index 000000000..af6cc2195 --- /dev/null +++ b/lazy.go @@ -0,0 +1,14 @@ +//go:build go1.21 + +package validator + +import ( + "regexp" + "sync" +) + +func lazyRegexCompile(str string) func() *regexp.Regexp { + return sync.OnceValue(func() *regexp.Regexp { + return regexp.MustCompile(str) + }) +} diff --git a/lazy_compat.go b/lazy_compat.go new file mode 100644 index 000000000..bd0bf4e94 --- /dev/null +++ b/lazy_compat.go @@ -0,0 +1,46 @@ +//go:build !go1.21 + +package validator + +import ( + "regexp" + "sync" +) + +// Copied and adapted from go1.21 stdlib's sync.OnceValue for backwards compatibility: +// OnceValue returns a function that invokes f only once and returns the value +// returned by f. The returned function may be called concurrently. +// +// If f panics, the returned function will panic with the same value on every call. +func onceValue(f func() *regexp.Regexp) func() *regexp.Regexp { + var ( + once sync.Once + valid bool + p interface{} + result *regexp.Regexp + ) + g := func() { + defer func() { + p = recover() + if !valid { + panic(p) + } + }() + result = f() + f = nil + valid = true + } + return func() *regexp.Regexp { + once.Do(g) + if !valid { + panic(p) + } + return result + } +} + +func lazyRegexCompile(str string) func() *regexp.Regexp { + return onceValue(func() *regexp.Regexp { + return regexp.MustCompile(str) + }) +} diff --git a/regexes.go b/regexes.go index f0dc25811..df0cfde71 100644 --- a/regexes.go +++ b/regexes.go @@ -1,10 +1,5 @@ package validator -import ( - "regexp" - "sync" -) - const ( alphaRegexString = "^[a-zA-Z]+$" alphaNumericRegexString = "^[a-zA-Z0-9]+$" @@ -79,12 +74,6 @@ const ( spicedbTypeRegexString = "^([a-z][a-z0-9_]{1,61}[a-z0-9]/)?[a-z][a-z0-9_]{1,62}[a-z0-9]$" ) -func lazyRegexCompile(str string) func() *regexp.Regexp { - return sync.OnceValue(func() *regexp.Regexp { - return regexp.MustCompile(str) - }) -} - var ( alphaRegex = lazyRegexCompile(alphaRegexString) alphaNumericRegex = lazyRegexCompile(alphaNumericRegexString) From dac5fe65522bc02695d7cacf05ecaca6e2fb9a85 Mon Sep 17 00:00:00 2001 From: Kimmo Lehto Date: Thu, 13 Jun 2024 10:38:05 +0300 Subject: [PATCH 3/3] Add tests for lazy regexp initialization Signed-off-by: Kimmo Lehto --- lazy_test.go | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 lazy_test.go diff --git a/lazy_test.go b/lazy_test.go new file mode 100644 index 000000000..4c7c74a17 --- /dev/null +++ b/lazy_test.go @@ -0,0 +1,83 @@ +package validator + +import ( + "regexp" + "sync" + "testing" +) + +// TestLazyRegexCompile_Basic tests that lazyRegexCompile compiles the regex only once and caches the result. +func TestLazyRegexCompile_Basic(t *testing.T) { + alphaRegexString := "^[a-zA-Z]+$" + alphaRegex := lazyRegexCompile(alphaRegexString) + + callCount := 0 + originalFunc := alphaRegex + alphaRegex = func() *regexp.Regexp { + callCount++ + return originalFunc() + } + + // Call the function multiple times + for i := 0; i < 10; i++ { + result := alphaRegex() + if result == nil { + t.Fatalf("Expected non-nil result") + } + if !result.MatchString("test") { + t.Fatalf("Expected regex to match 'test'") + } + } + + if callCount != 10 { + t.Fatalf("Expected call count to be 10, got %d", callCount) + } +} + +// TestLazyRegexCompile_Concurrent tests that lazyRegexCompile works correctly when called concurrently. +func TestLazyRegexCompile_Concurrent(t *testing.T) { + alphaRegexString := "^[a-zA-Z]+$" + alphaRegex := lazyRegexCompile(alphaRegexString) + + var wg sync.WaitGroup + const numGoroutines = 100 + + // Use a map to ensure all results point to the same instance + results := make(map[*regexp.Regexp]bool) + var mu sync.Mutex + + // Call the function concurrently + for i := 0; i < numGoroutines; i++ { + wg.Add(1) + go func() { + defer wg.Done() + result := alphaRegex() + if result == nil { + t.Errorf("Expected non-nil result") + } + mu.Lock() + results[result] = true + mu.Unlock() + }() + } + wg.Wait() + + if len(results) != 1 { + t.Fatalf("Expected one unique regex instance, got %d", len(results)) + } +} + +// TestLazyRegexCompile_Panic tests that if the regex compilation panics, the panic value is propagated consistently. +func TestLazyRegexCompile_Panic(t *testing.T) { + faultyRegexString := "[a-z" + alphaRegex := lazyRegexCompile(faultyRegexString) + + defer func() { + if r := recover(); r == nil { + t.Fatalf("Expected a panic, but none occurred") + } + }() + + // Call the function, which should panic + alphaRegex() +}