Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Data race in tests #35

Open
hagaibarel opened this issue Aug 6, 2019 · 1 comment
Open

Data race in tests #35

hagaibarel opened this issue Aug 6, 2019 · 1 comment

Comments

@hagaibarel
Copy link

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

@jjjordanmsft
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants