ApplicationInsights-Go icon indicating copy to clipboard operation
ApplicationInsights-Go copied to clipboard

Data race in tests

Open hagaibarel opened this issue 5 years ago • 1 comments

Hi,

Seems there's a data race. Running go test --race ./appinsights/... produces the following:

==================
WARNING: DATA RACE
Write at 0x000000cc64e0 by goroutine 27:
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*diagnosticsMessageWriter).appendListener()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/diagnostics.go:50 +0xed
  github.com/microsoft/ApplicationInsights-Go/appinsights.TestMessageSentToConsumers()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/diagnostics.go:43 +0x1cf
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163

Previous read at 0x000000cc64e0 by goroutine 17:
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*httpTransmitter).Transmit()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/diagnostics.go:87 +0x9d9
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*InMemoryChannel).transmitRetry()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/inmemorychannel.go:370 +0x23b
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*inMemoryChannelState).send.func1()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/inmemorychannel.go:289 +0xbd

Goroutine 27 (running) created at:
  testing.(*T).Run()
      /usr/local/go/src/testing/testing.go:916 +0x65a
  testing.runTests.func1()
      /usr/local/go/src/testing/testing.go:1157 +0xa8
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:865 +0x163
  testing.runTests()
      /usr/local/go/src/testing/testing.go:1155 +0x523
  testing.(*M).Run()
      /usr/local/go/src/testing/testing.go:1072 +0x2eb
  main.main()
      _testmain.go:136 +0x222

Goroutine 17 (finished) created at:
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*inMemoryChannelState).send()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/inmemorychannel.go:287 +0x1d2
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*inMemoryChannelState).waitToSend()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/inmemorychannel.go:264 +0x7d4
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*inMemoryChannelState).start()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/inmemorychannel.go:210 +0x384
  github.com/microsoft/ApplicationInsights-Go/appinsights.(*InMemoryChannel).acceptLoop()
      /home/hbarel/go/src/github.com/microsoft/ApplicationInsights-Go/appinsights/inmemorychannel.go:146 +0x4d
==================
--- FAIL: TestMessageSentToConsumers (0.00s)
    testing.go:809: race detected during execution of test
FAIL
FAIL    github.com/microsoft/ApplicationInsights-Go/appinsights 4.022s
?       github.com/microsoft/ApplicationInsights-Go/appinsights/contracts       [no test files]

Introducing a writer.lock.Lock() (and a defer unlock of course) to diagnostics.go:87 in the hasListeners() function seems to resolve it

hagaibarel avatar Aug 06 '19 10:08 hagaibarel

I wasn't a fan of locking the diagnostics listeners (it may constitute a race condition, but I don't think errors could be induced from this) when I implemented it, but I think a reader-writer lock would be acceptable.

jjjordanmsft avatar Aug 09 '19 23:08 jjjordanmsft