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 data race in matching test suite #5781

Conversation

taylanisikdemir
Copy link
Member

What changed?
ensureAsyncReady in matching test suite was causing data races because it's running the given callback function in a separate goroutine that accesses shared variables.
This was consistently failing in one of the buildkite jobs I ran in a separate internal fork so addressing it by making following changes:

  • add mutex around shared variable access
  • ensure the returned wait doesn't return before the goroutine terminates

It's not a simple solution but gets the job done. That whole suite could use some refactoring to separate out running Poll/Offer goroutines and the test validation.

Why?
Fix test data race.

How did you test it?
go test -timeout 120s -run ^TestMatcherSuite$ github.com/uber/cadence/service/matching -v -count=10 -race

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

Merging #5781 (15aa30f) into master (56175e1) will decrease coverage by 0.02%.
The diff coverage is n/a.

❗ Current head 15aa30f differs from pull request most recent head 067486f. Consider uploading reports for the commit 067486f to get more accurate results

Additional details and impacted files

see 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56175e1...067486f. Read the comment docs.

@coveralls
Copy link

coveralls commented Mar 13, 2024

Pull Request Test Coverage Report for Build 018e38cb-7382-468b-b91e-9acbde2fade7

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 67 unchanged lines in 15 files lost coverage.
  • Overall coverage decreased (-0.02%) to 64.837%

Files with Coverage Reduction New Missed Lines %
service/history/queue/timer_queue_processor_base.go 1 77.62%
service/history/task/transfer_standby_task_executor.go 2 86.19%
common/task/fifo_task_scheduler.go 2 85.57%
common/persistence/sql/sqlplugin/mysql/task.go 2 73.68%
service/matching/db.go 2 73.23%
service/history/task/fetcher.go 2 85.05%
common/persistence/sql/sqlplugin/mysql/db.go 2 79.49%
common/log/tag/tags.go 3 50.18%
service/history/queue/timer_gate.go 3 95.83%
common/persistence/nosql/nosqlplugin/cassandra/workflow.go 3 71.41%
Totals Coverage Status
Change from base Build 018e388c-41e9-4b4a-8784-d679ef4f68ac: -0.02%
Covered Lines: 94506
Relevant Lines: 145760

💛 - Coveralls

@taylanisikdemir taylanisikdemir enabled auto-merge (squash) March 13, 2024 17:10
@taylanisikdemir taylanisikdemir merged commit 3c7c303 into cadence-workflow:master Mar 13, 2024
18 checks passed
@taylanisikdemir taylanisikdemir deleted the taylan/matching_test_race branch March 13, 2024 17:38
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

Successfully merging this pull request may close these issues.

3 participants