Data Race in Testing

[MrAlias]

A recent run of the testing suite revealed a data race in the stackdriver exporter unit tests.

==================
WARNING: DATA RACE
Read at 0x00000128e9d8 by goroutine 25:
  go.opentelemetry.io/otel/exporter/trace/stackdriver_test.(*mockTraceServer).BatchWriteSpans()
      /home/circleci/project/exporter/trace/stackdriver/stackdriver_test.go:52 +0x1b1
  google.golang.org/genproto/googleapis/devtools/cloudtrace/v2._TraceService_BatchWriteSpans_Handler()
      /go/pkg/mod/google.golang.org/genproto@v0.0.0-20191009194640-548a555dbc03/googleapis/devtools/cloudtrace/v2/tracing.pb.go:196 +0x2fc
  google.golang.org/grpc.(*Server).processUnaryRPC()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/server.go:995 +0x994
  google.golang.org/grpc.(*Server).handleStream()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/server.go:1275 +0x1343
  google.golang.org/grpc.(*Server).serveStreams.func1.1()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/server.go:710 +0xc8

Previous write at 0x00000128e9d8 by goroutine 24:
  go.opentelemetry.io/otel/exporter/trace/stackdriver_test.TestExporter_Timeout()
      /home/circleci/project/exporter/trace/stackdriver/stackdriver_test.go:127 +0x52
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199

Goroutine 25 (running) created at:
  google.golang.org/grpc.(*Server).serveStreams.func1()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/server.go:708 +0xb8
  google.golang.org/grpc/internal/transport.(*http2Server).operateHeaders()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/internal/transport/http2_server.go:434 +0x169c
  google.golang.org/grpc/internal/transport.(*http2Server).HandleStreams()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/internal/transport/http2_server.go:475 +0x3d7
  google.golang.org/grpc.(*Server).serveStreams()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/server.go:706 +0x19a
  google.golang.org/grpc.(*Server).handleRawConn.func1()
      /go/pkg/mod/google.golang.org/grpc@v1.24.0/server.go:668 +0x4c

Goroutine 24 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:960 +0x651
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1202 +0xa6
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:909 +0x199
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1200 +0x521
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1117 +0x2ff
  go.opentelemetry.io/otel/exporter/trace/stackdriver_test.TestMain()
      /home/circleci/project/exporter/trace/stackdriver/stackdriver_test.go:92 +0x3e2
  main.main()
      _testmain.go:98 +0x333
==================

A quick look at the code shows the two lines of the mentioned file show that the contention comes from testing directly setting fields of the mock trace server:

• https://github.com/open-telemetry/opentelemetry-go/blob/master/exporter/trace/stackdriver/stackdriver_test.go#L97-L98
• https://github.com/open-telemetry/opentelemetry-go/blob/master/exporter/trace/stackdriver/stackdriver_test.go#L127-L128

This is in contention with the mock tracer server's own method and resulting in the data race.

Guards around these concurrent values need to be put in place and the unit tests need to be updated to not directly access them.

[paivagustavo]

Could this be an opportunity to ask to move proprietary exporters out of the main repo?

[MrAlias]

Created a ticket to remove vendor exporters. That issue closes this one if it itself is closed.