Test-scoped data race in GRPC tests

[roobre]

There seems to be a data race in the GRPC proxy tests, that only triggers under unknown circumstances:

go test -race  ./...
?   	github.com/grafana/xk6-disruptor/cmd/agent	[no test files]
?   	github.com/grafana/xk6-disruptor/cmd/e2e-cluster	[no test files]
?   	github.com/grafana/xk6-disruptor/cmd/agent/commands	[no test files]
?   	github.com/grafana/xk6-disruptor/cmd/e2e-cluster/commands	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/internal/version	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/kubernetes	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/runtime/profiler	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/assertions	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/checks	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/cluster	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/deploy	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/fetch	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/fixtures	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/kubectl	[no test files]
?   	github.com/grafana/xk6-disruptor/pkg/testutils/e2e/kubernetes	[no test files]
ok  	github.com/grafana/xk6-disruptor	1.087s
?   	github.com/grafana/xk6-disruptor/pkg/testutils/grpc	[no test files]
ok  	github.com/grafana/xk6-disruptor/pkg/agent	6.013s
ok  	github.com/grafana/xk6-disruptor/pkg/agent/protocol	1.008s
==================
WARNING: DATA RACE
Write at 0x00c0001b81f0 by goroutine 14:
  github.com/grafana/xk6-disruptor/pkg/agent/protocol/grpc.(*proxy).Start()
      /home/roobre/Devel/xk6-disruptor/pkg/agent/protocol/grpc/proxy.go:104 +0x4f0
  github.com/grafana/xk6-disruptor/pkg/agent/protocol/grpc.Test_ProxyMetrics.func1.3()
      /home/roobre/Devel/xk6-disruptor/pkg/agent/protocol/grpc/proxy_test.go:439 +0x46

Previous read at 0x00c0001b81f0 by goroutine 10:
  github.com/grafana/xk6-disruptor/pkg/agent/protocol/grpc.(*proxy).Stop()
      /home/roobre/Devel/xk6-disruptor/pkg/agent/protocol/grpc/proxy.go:123 +0x34
  github.com/grafana/xk6-disruptor/pkg/agent/protocol/grpc.Test_ProxyMetrics.func1.2()
      /home/roobre/Devel/xk6-disruptor/pkg/agent/protocol/grpc/proxy_test.go:435 +0x39
  runtime.deferreturn()
      /usr/lib/go/src/runtime/panic.go:477 +0x30
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:1595 +0x238
  testing.(*T).Run.func1()
      /usr/lib/go/src/testing/testing.go:1648 +0x44

[...]

The problem seems to be that the start function is run on a goroutine:

xk6-disruptor/pkg/agent/protocol/grpc/proxy_test.go

Lines 438 to 442 in daa6376

	go func() {
	if perr := proxy.Start(); perr != nil {
	t.Logf("error starting proxy: %v", perr)
	}
	}()

While at the same time, Stop is called on a deferred function:

xk6-disruptor/pkg/agent/protocol/grpc/proxy_test.go

Lines 434 to 436 in daa6376

	defer func() {
	_ = proxy.Stop()
	}()

If the test ends prematurely, the Stop function attempts to read and then act upon p.srv, which Start also modifies.

xk6-disruptor/pkg/agent/protocol/grpc/proxy.go

Lines 122 to 127 in daa6376

	func (p *proxy) Stop() error {
	if p.srv != nil {
	p.srv.GracefulStop()
	}
	return nil
	}

Fixing this externally is likely not possible, as it is not possible to know when Start has finished initialization. and thus it is safe to call Stop. A solution could be to protect reads/writes from/to p.srv behind a mutex.

[pablochacin]

A mutex seems a sensible solution. We just must be careful because Start is a blocking function, so we can't just defer releasing the lock. Instead we need to ensure it is always released before starting to listen for connections.