Add helper to create a sub-registry or return the existing one if present (#313)

When creating a child registry, code often checks if the registry already exists, since creating a duplicate entry will cause a panic. There are two issues with this:

- Most registry creation has extra identical boilerplate to check if a registry exists before creating it
- This boilerplate is not concurrency-safe, because the registry lock is not held continuously; it's possible for two goroutines to both check the same registry name, both observe that it does not exist yet, and both try to create it. This is rare (I don't know of confirmed occurrences) but if it does happen the entire process will panic.

This PR adds a helper that handles both these issues, replacing the usual checks with a one-line call that properly holds the lock between checking for existing registries and creating new ones.

Author: faec

monitoring/registry.go

@@ -106,22 +106,52 @@ func (r *Registry) Get(name string) Var {
 
 // GetRegistry tries to find a sub-registry by name.
 func (r *Registry) GetRegistry(name string) *Registry {
-	e, err := r.find(name)
-	if err != nil {
-		return nil
+	if v := r.Get(name); v != nil {
+		if reg, ok := v.(*Registry); ok {
+			return reg
+		}
 	}
+	return nil
+}
 
-	v := e.Var
-	if v == nil {
-		return nil
+// Creates new recursive registries under the given sequence of names,
+// returning the final leaf node.
+// r.mu must be held by the caller.
+func (r *Registry) newRegistryChainWithLock(names []string, opts *options) *Registry {
+	cur := r
+	for _, name := range names {
+		sub := &Registry{
+			name:    fullName(cur, name),
+			opts:    opts,
+			entries: map[string]entry{},
+		}
+		cur.entries[name] = entry{sub, sub.opts.mode}
+		cur = sub
 	}
+	return cur
+}
 
-	reg, ok := v.(*Registry)
-	if !ok {
-		return nil
+func (r *Registry) GetOrCreateRegistry(name string, opts ...Option) *Registry {
+	names := strings.Split(name, ".")
+	return r.getOrCreateRegistry(names, opts...)
+}
+
+func (r *Registry) getOrCreateRegistry(names []string, opts ...Option) *Registry {
+	if len(names) == 0 {
+		return r
 	}
+	name := names[0]
+	r.mu.Lock()
+	defer r.mu.Unlock()
 
-	return reg
+	if entry, found := r.entries[name]; found {
+		reg, ok := entry.Var.(*Registry)
+		if !ok {
+			return nil
+		}
+		return reg.getOrCreateRegistry(names[1:], opts...)
+	}
+	return r.newRegistryChainWithLock(names, applyOpts(r.opts, opts))
 }
 
 // Remove removes a variable or a sub-registry by name

monitoring/registry_test.go

@@ -155,3 +155,26 @@ func TestRegistryIter(t *testing.T) {
 
 	assert.Equal(t, vars, collected)
 }
+
+func TestGetOrCreateRegistry(t *testing.T) {
+	root := &Registry{
+		name:    "root",
+		entries: map[string]entry{},
+		opts:    &defaultOptions,
+	}
+
+	require.Nil(t, root.GetRegistry("a.b.c"), "GetRegistry on empty registry always returns nil")
+
+	c := root.GetOrCreateRegistry("a.b.c")
+	require.NotNil(t, c, "GetOrCreateRegistry must be successful on an empty registry")
+	assert.Equal(t, c, root.GetRegistry("a.b.c"), "GetRegistry after GetOrCreateRegistry should return the same value")
+	assert.Equal(t, "root.a.b.c", c.name, "Registries created with GetOrCreateRegistry should contain the parent name followed by the path to the registry")
+
+	y := c.GetOrCreateRegistry("z.y")
+	require.NotNil(t, y, "GetOrCreateRegistry must be successful on an empty registry")
+	assert.Equal(t, y, c.GetOrCreateRegistry("z.y"), "GetOrCreateRegistry on the same input should return the same result")
+	assert.Equal(t, c.GetOrCreateRegistry("z.y"), root.GetOrCreateRegistry("a.b.c.z.y"), "GetOrCreateRegistry with equivalent paths from different starting points should return the same result")
+
+	c.Add("scalar", &Int{}, Full)
+	assert.Nil(t, root.GetOrCreateRegistry("a.b.c.scalar.w.x"), "GetOrCreateRegistry should return nil if part of the path is a non-registry type")
+}