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

fix: race condition for err variable when statsd start is called #44

Merged
merged 1 commit into from
May 8, 2023

Conversation

gane5hvarma
Copy link
Contributor

@gane5hvarma gane5hvarma commented May 7, 2023

Description

Before this change, the error variable declared in the start function syntax is modified by the goroutine at line 48. Since we are returning nil at the end, err variable is set to nil. leading to a potential data race by main routine and go routine
Test this in go playground
I got to know about it when I was testing this pr for a race condition.
These are the logs, i got when i ran rudder-sources in race condition

==================
WARNING: DATA RACE
Write at 0x00c0004884f0 by main goroutine:
  github.com/rudderlabs/rudder-go-kit/stats.(*statsdStats).Start()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/vendor/github.com/rudderlabs/rudder-go-kit/stats/statsd.go:69 +0x5b8
  github.com/rudderlabs/rudder-sources/metrics/stats.Setup()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/metrics/stats/stats.go:27 +0x254
  github.com/rudderlabs/rudder-sources/app.(*App).initMetrics()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/app/app.go:136 +0x6c
  github.com/rudderlabs/rudder-sources/app.New()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/app/app.go:48 +0x240
  main.main()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/cmd/rudder-sources/main.go:143 +0xdec

Previous read at 0x00c0004884f0 by goroutine 32:
  github.com/rudderlabs/rudder-go-kit/stats.(*statsdStats).Start.func1()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/vendor/github.com/rudderlabs/rudder-go-kit/stats/statsd.go:45 +0x5c

Goroutine 32 (running) created at:
  github.com/rudderlabs/rudder-go-kit/stats.defaultGoRoutineFactory.Go()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/vendor/github.com/rudderlabs/rudder-go-kit/stats/stats.go:141 +0x28
  github.com/rudderlabs/rudder-go-kit/stats.(*defaultGoRoutineFactory).Go()
      <autogenerated>:1 +0x3c
  github.com/rudderlabs/rudder-go-kit/stats.(*statsdStats).Start()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/vendor/github.com/rudderlabs/rudder-go-kit/stats/statsd.go:44 +0x4b0
  github.com/rudderlabs/rudder-sources/metrics/stats.Setup()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/metrics/stats/stats.go:27 +0x254
  github.com/rudderlabs/rudder-sources/app.(*App).initMetrics()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/app/app.go:136 +0x6c
  github.com/rudderlabs/rudder-sources/app.New()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/app/app.go:48 +0x240
  main.main()
      /Users/ganeshvarmaraghavaraju/rudderstack/rudder-sources/cmd/rudder-sources/main.go:143 +0xdec
==================

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@github-actions github-actions bot added the stats label May 7, 2023
@codecov
Copy link

codecov bot commented May 7, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (03bc460) 77.26% compared to head (bf37c0a) 77.27%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #44   +/-   ##
=======================================
  Coverage   77.26%   77.27%           
=======================================
  Files          48       48           
  Lines        3167     3168    +1     
=======================================
+ Hits         2447     2448    +1     
  Misses        595      595           
  Partials      125      125           
Impacted Files Coverage Δ
stats/statsd.go 74.01% <100.00%> (+0.14%) ⬆️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@atzoum atzoum changed the title fix race condition for err variable when statsd start is called fix: race condition for err variable when statsd start is called May 7, 2023
@fracasula fracasula merged commit 5702038 into main May 8, 2023
@fracasula fracasula deleted the chore.fixDataRaceInStatsd branch May 8, 2023 07:10
@github-actions github-actions bot mentioned this pull request May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants