[Bug] Race condition on monitoring.New[Uint, Int, ...]

[AndersonQ]

The function to create new variables on a registry, like monitoring.NewInt, monitoring.NewUint and so on have a race condition when called concurrent to create a variable of the same name.

Those functions try to fetch en existing variable, if none is found, they add a new variable to the registry. However the check for the existence of the variable and the creation when none is found isn't done atomically. Thus it happens 2 calls for the same variable name identify the variable do not exists, both try to add it and the second panics.

For examples, NewUint:

elastic-agent-libs/monitoring/metrics.go

Lines 79 to 80 in d2796b9

	func NewUint(r *Registry, name string, opts ...Option) *Uint {
	existingVar, r := setupMetric(r, name, opts)

it calls Get:

elastic-agent-libs/monitoring/metrics.go

Lines 336 to 343 in d2796b9

	func setupMetric(reg *Registry, name string, opts []Option) (Var, *Registry) {
	if reg == nil {
	return nil, Default
	}
	vis := reg.Get(name)
	if vis == nil {
	return nil, reg
	}

and if none is found, it adds a new one:

elastic-agent-libs/monitoring/metrics.go

Lines 90 to 95 in d2796b9

	v := &Uint{}
	addVar(r, name, opts, v, makeExpvar(func() string {
	return strconv.FormatUint(v.Get(), 10)
	}))
	return v
	}

However there is no synchronisation between the call to Get and to addVar. Thus it happens:

• goroutine A: calls NewUint, which calls Get and there is no variable
• goroutine B: calls NewUint, which calls Get and there is no variable
• goroutine B: NewUint calls addVar and the new variable is created
• goroutine A: NewUint calls addVar and, as goroutine B already created the variable, addVar panics

To reproduce the issue, add the following test and run it multiple times:

diff --git a/monitoring/registry_test.go b/monitoring/registry_test.go
index c9fdeca..6be4934 100644
--- a/monitoring/registry_test.go
+++ b/monitoring/registry_test.go
@@ -20,6 +20,7 @@
 package monitoring

 import (
+       "sync"
        "testing"

        "github.com/stretchr/testify/assert"
@@ -103,6 +104,23 @@ func TestRegistryGet(t *testing.T) {
        assert.Equal(t, v, v3)
 }

+func TestRegistryGet_concurrent(t *testing.T) {
+       reg := NewRegistry()
+       name := "foo"
+
+       wg := sync.WaitGroup{}
+       assert.NotPanics(t, func() {
+               for i := 0; i < 5; i++ {
+                       wg.Add(1)
+                       go func() {
+                               defer wg.Done()
+                               NewInt(reg, name)
+                       }()
+               }
+       })
+       wg.Wait()
+}
+
 func TestRegistryRemove(t *testing.T) {
        defer func(t *testing.T) {
                err := Clear()

go test -run TestRegistryGet_concurrent -count 10000 .

[mauri870]

I'm not sure this is a race condition. The code around the panic appears to be properly synchronized, and the race detector doesn't report anything. It seems more like the panic is acting as a guard, based on the assumption that creating a variable with the same name was inherently wrong.

But to me, it looks perfectly valid to create a new variable with the same name—it should simply replace the old one. If this is not desired, then perhaps we should return an error instead.

 func (r *Registry) addNames(names []string, v Var, opts *options) error {
        r.mu.Lock()
        defer r.mu.Unlock()

        name := names[0]
        if len(names) == 1 {
-               if _, found := r.entries[name]; found {
-                       return fmt.Errorf("name %v already used", name)
-               }
-
                r.entries[name] = entry{v, opts.mode}
                return nil
        }

        if tmp, found := r.entries[name]; found {
                reg, ok := tmp.Var.(*Registry)
                if !ok {
                        return fmt.Errorf("name %v already used", name)
                }

                return reg.addNames(names[1:], v, opts)
        }

        sub := NewRegistry()
        sub.opts = opts
        if err := sub.addNames(names[1:], v, opts); err != nil {
                return err
        }

        r.entries[name] = entry{sub, sub.opts.mode}
        return nil
 }

[AndersonQ]

@mauri870 as I said in the issue description the problem happens because the check for existing variables and the creation when one does not exist isn't synchronised.

For examples, NewUint:

elastic-agent-libs/monitoring/metrics.go

Lines 79 to 80 in d2796b9

	func NewUint(r *Registry, name string, opts ...Option) *Uint {
	existingVar, r := setupMetric(r, name, opts)

it calls Get:

elastic-agent-libs/monitoring/metrics.go

Lines 336 to 343 in d2796b9

	func setupMetric(reg *Registry, name string, opts []Option) (Var, *Registry) {
	if reg == nil {
	return nil, Default
	}
	vis := reg.Get(name)
	if vis == nil {
	return nil, reg
	}

and if none is found, it adds a new one:

elastic-agent-libs/monitoring/metrics.go

Lines 90 to 95 in d2796b9

	v := &Uint{}
	addVar(r, name, opts, v, makeExpvar(func() string {
	return strconv.FormatUint(v.Get(), 10)
	}))
	return v
	}

However there is no synchronisation between the call to Get and to addVar. Thus it happens:

• goroutine A: calls NewUint, which calls Get and there is no variable
• goroutine B: calls NewUint, which calls Get and there is no variable
• goroutine B: NewUint calls addVar and the new variable is created
• goroutine A: NewUint calls addVar and, as goroutine B already created the variable, addVar panics

[mauri870]

Ah, I see it now, thanks for explaining.