search

digitalocean / firebolt

Golang framework for streaming ETL, observability data pipeline, and event processing apps
Other
698 stars 50 forks source link

DATA RACE in node/elasticsearch/connectionfactory.go #25

Closed cristovarghese closed 3 years ago

cristovarghese commented 3 years ago

Generate an app binary with race detector which uses the elasticsearch bulkindexer async node in the firebolt yaml config. Run it and you should be able to see the following race condition. The fix seems to be pretty simple in node/elasticsearch/connectionfactory.go, we can just use the results from atomic.AddInt64() operation instead of referencing e.batchCount.

If this fix seems OK, I can submit a PR.


==================
WARNING: DATA RACE
Write at 0x00c000110070 by goroutine 93:
  sync/atomic.AddInt64()
      src/runtime/race_amd64.s:276 +0xb
  github.com/digitalocean/firebolt/node/elasticsearch.(*esBulkServiceFactory).BulkService()
      external/com_github_digitalocean_firebolt/node/elasticsearch/connectionfactory.go:58 +0x72
  github.com/digitalocean/firebolt/node/elasticsearch.(*ElasticIndexClient).doBulkIndex()
      external/com_github_digitalocean_firebolt/node/elasticsearch/elastic_index_client.go:164 +0x8f
  github.com/digitalocean/firebolt/node/elasticsearch.(*ElasticIndexClient).retryBulkIndex.func1()
      external/com_github_digitalocean_firebolt/node/elasticsearch/elastic_index_client.go:139 +0x141

Previous read at 0x00c000110070 by goroutine 92:
  [failed to restore the stack]

Goroutine 93 (running) created at:
  github.com/digitalocean/firebolt/node/elasticsearch.(*ElasticIndexClient).retryBulkIndex()
      external/com_github_digitalocean_firebolt/node/elasticsearch/elastic_index_client.go:133 +0x146
  github.com/digitalocean/firebolt/node/elasticsearch.(*ElasticIndexClient).batch()
      external/com_github_digitalocean_firebolt/node/elasticsearch/elastic_index_client.go:114 +0x310

Goroutine 92 (running) created at:
  github.com/digitalocean/firebolt/node/elasticsearch.(*ElasticIndexClient).retryBulkIndex()
      external/com_github_digitalocean_firebolt/node/elasticsearch/elastic_index_client.go:133 +0x146
  github.com/digitalocean/firebolt/node/elasticsearch.(*ElasticIndexClient).batch()
      external/com_github_digitalocean_firebolt/node/elasticsearch/elastic_index_client.go:114 +0x310
==================
jnadler commented 3 years ago

Thanks for this report! If you have time to submit a PR for this issue, I can review it on Monday.

cristovarghese commented 3 years ago

I dont seem to have the permissions to submit a PR for this repo.

`ERROR: Permission to digitalocean/firebolt.git denied to cristovarghese. fatal: Could not read from remote repository.

Please make sure you have the correct access rights and the repository exists.`

jnadler commented 3 years ago

You should have permissions for that. First, commit and push the change to your https://github.com/cristovarghese/firebolt fork (I see it doesn't have any commits yet). Then, from that fork, create a PR back to digitalocean/firebolt.

cristovarghese commented 3 years ago

Thanks, have opened a PR for it.

jnadler commented 3 years ago

Thank you @cristovarghese! Closed with #26