fix: another config deadlock by trying to acquire a read lock twice (#…

…120)

config/config.go

@@ -305,7 +305,7 @@ func getReloadableMapKeys[T configTypes](v T, orderedKeys ...string) (string, st
 func getOrCreatePointer[T configTypes](
 	m map[string]any, dvs map[string]string, // this function MUST receive maps that are already initialized
 	lock *sync.RWMutex, defaultValue T, orderedKeys ...string,
-) *Reloadable[T] {
+) (ptr *Reloadable[T], exists bool) {
 	key, dvKey := getReloadableMapKeys(defaultValue, orderedKeys...)
 
 	lock.Lock()
@@ -324,12 +324,12 @@ func getOrCreatePointer[T configTypes](
 	}()
 
 	if p, ok := m[key]; ok {
-		return p.(*Reloadable[T])
+		return p.(*Reloadable[T]), true
 	}
 
 	p := &Reloadable[T]{}
 	m[key] = p
-	return p
+	return p, false
 }
 
 // bindEnv handles rudder server's unique snake case replacement by registering

config/config_test.go

@@ -1,11 +1,14 @@
 package config
 
 import (
+	"context"
+	"fmt"
 	"sync"
 	"testing"
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"golang.org/x/sync/errgroup"
 )
 
 func Test_Getters_Existing_and_Default(t *testing.T) {
@@ -437,17 +440,21 @@ func TestGetOrCreatePointer(t *testing.T) {
 		dvs = make(map[string]string)
 		rwm sync.RWMutex
 	)
-	p1 := getOrCreatePointer(m, dvs, &rwm, 123, "foo", "bar")
+	p1, exists := getOrCreatePointer(m, dvs, &rwm, 123, "foo", "bar")
 	require.NotNil(t, p1)
+	require.False(t, exists)
 
-	p2 := getOrCreatePointer(m, dvs, &rwm, 123, "foo", "bar")
+	p2, exists := getOrCreatePointer(m, dvs, &rwm, 123, "foo", "bar")
 	require.True(t, p1 == p2)
+	require.True(t, exists)
 
-	p3 := getOrCreatePointer(m, dvs, &rwm, 123, "bar", "foo")
+	p3, exists := getOrCreatePointer(m, dvs, &rwm, 123, "bar", "foo")
 	require.True(t, p1 != p3)
+	require.False(t, exists)
 
-	p4 := getOrCreatePointer(m, dvs, &rwm, 123, "bar", "foo", "qux")
+	p4, exists := getOrCreatePointer(m, dvs, &rwm, 123, "bar", "foo", "qux")
 	require.True(t, p1 != p4)
+	require.False(t, exists)
 
 	require.PanicsWithError(t,
 		"Detected misuse of config variable registered with different default values "+
@@ -586,3 +593,81 @@ func Test_Misc(t *testing.T) {
 	t.Setenv("RELEASE_NAME", "value")
 	require.Equal(t, "value", GetReleaseName())
 }
+
+func TestConfigLocking(t *testing.T) {
+	const (
+		timeout   = 2 * time.Second
+		configKey = "test"
+	)
+	c := New()
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+	g, ctx := errgroup.WithContext(ctx)
+
+	doWithTimeout := func(f func(), timeout time.Duration) error {
+		var err error
+		var closed bool
+		var closedMutex sync.Mutex
+		wait := make(chan struct{})
+		t := time.NewTimer(timeout)
+
+		go func() {
+			<-t.C
+			err = fmt.Errorf("timeout after %s", timeout)
+			closedMutex.Lock()
+			if !closed {
+				closed = true
+				close(wait)
+			}
+			closedMutex.Unlock()
+		}()
+
+		go func() {
+			f()
+			t.Stop()
+			closedMutex.Lock()
+			if !closed {
+				closed = true
+				close(wait)
+			}
+			closedMutex.Unlock()
+		}()
+		<-wait
+		return err
+	}
+
+	startOperation := func(name string, op func()) {
+		g.Go(func() error {
+			for {
+				select {
+				case <-ctx.Done():
+					return nil
+				default:
+					if err := doWithTimeout(op, timeout); err != nil {
+						return fmt.Errorf("%s: %w", name, err)
+					}
+				}
+			}
+		})
+	}
+
+	startOperation("set the config value", func() { c.Set(configKey, "value1") })
+	startOperation("try to read the config value using GetString", func() { _ = c.GetString(configKey, "") })
+	startOperation("try to read the config value using GetStringVar", func() { _ = c.GetStringVar(configKey, "") })
+	startOperation("try to read the config value using GetReloadableStringVar", func() {
+		r := c.GetReloadableStringVar("", configKey)
+		_ = r.Load()
+	})
+
+	g.Go(func() error {
+		select {
+		case <-ctx.Done():
+			return nil
+		case <-time.After(5 * time.Second):
+			cancel()
+			return nil
+		}
+	})
+
+	require.NoError(t, g.Wait())
+}