allow duplicate env.Get registrations (#45871)

In the upcoming single-program mode, multiple services in the same process will call the same env.Get lines. This is safe as long as they register the same default value and description for a given env var name. The alternative would be to ensure that env.Get is only called a single time for each env var name, which would require a lot of pointless "is initialized?" checks and lead to more complexity than this simple change.

Also adds tests for env.Get.

internal/env/env.go

@@ -16,14 +16,17 @@ import (
 )
 
 type envflag struct {
-	name        string
 	description string
 	value       string
 }
 
-var env []envflag
-var environ map[string]string
-var locked = false
+var (
+	env     map[string]envflag
+	environ map[string]string
+	locked  = false
+
+	expvarPublish = true
+)
 
 var (
 	// MyName represents the name of the current process.
@@ -119,11 +122,15 @@ func Get(name, defaultValue, description string) string {
 		}
 	}
 
-	env = append(env, envflag{
-		name:        name,
-		description: description,
-		value:       value,
-	})
+	if env == nil {
+		env = map[string]envflag{}
+	}
+
+	e := envflag{description: description, value: value}
+	if existing, ok := env[name]; ok && existing != e {
+		panic(fmt.Sprintf("env var %q already registered with a different description or value", name))
+	}
+	env[name] = e
 
 	return value
 }
@@ -175,24 +182,26 @@ func Lock() {
 
 	locked = true
 
-	sort.Slice(env, func(i, j int) bool { return env[i].name < env[j].name })
-
-	for i := 1; i < len(env); i++ {
-		if env[i-1].name == env[i].name {
-			panic(fmt.Sprintf("%q already registered", env[i].name))
-		}
+	if expvarPublish {
+		expvar.Publish("env", expvar.Func(func() any {
+			return env
+		}))
 	}
-
-	expvar.Publish("env", expvar.Func(func() any {
-		return env
-	}))
 }
 
 // HelpString prints a list of all registered environment variables and their descriptions.
 func HelpString() string {
 	helpStr := "Environment variables:\n"
-	for _, e := range env {
-		helpStr += fmt.Sprintf("  %-40s %s (value: %q)\n", e.name, e.description, e.value)
+
+	sorted := make([]string, 0, len(env))
+	for name := range env {
+		sorted = append(sorted, name)
+	}
+	sort.Strings(sorted)
+
+	for _, name := range sorted {
+		e := env[name]
+		helpStr += fmt.Sprintf("  %-40s %s (value: %q)\n", name, e.description, e.value)
 	}
 
 	return helpStr

internal/env/env_test.go

@@ -22,7 +22,47 @@ func TestEnvironMap(t *testing.T) {
 }
 
 func TestLock(t *testing.T) {
-	// Test that calling lock won't panic. This will be the only caller for
-	// Lock in our test.
+	// Test that calling lock won't panic.
 	Lock()
 }
+
+func TestGet(t *testing.T) {
+	reset := func(osEnviron map[string]string) {
+		env = nil
+		environ = osEnviron
+		locked = false
+		expvarPublish = false // avoid "Reuse of exported var name" panic from package expvar
+	}
+	t.Cleanup(func() { reset(nil) })
+
+	t.Run("normal", func(t *testing.T) {
+		reset(map[string]string{"B": "z"})
+
+		a := Get("A", "x", "foo")
+		b := Get("B", "y", "bar")
+		b2 := Get("B", "y", "bar")
+		Lock()
+		if want := "x"; a != want {
+			t.Errorf("got A == %q, want %q", a, want)
+		}
+		if want := "z"; b != want {
+			t.Errorf("got B == %q, want %q", b, want)
+		}
+		if want := "z"; b2 != want {
+			t.Errorf("got B2 == %q, want %q", b2, want)
+		}
+	})
+
+	t.Run("conflicting registrations", func(t *testing.T) {
+		reset(nil)
+
+		Get("A", "x", "foo")
+		defer func() {
+			if e := recover(); e == nil {
+				t.Error("want panic")
+			}
+		}()
+		Get("A", "y", "bar")
+		t.Error("want panic")
+	})
+}