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

progress: fix incorrect mutex usages #154

Merged
merged 2 commits into from
Apr 7, 2021
Merged

Conversation

virtuald
Copy link
Contributor

@virtuald virtuald commented Apr 4, 2021

  • tracker.mutexPrv is useless since it only protects writes. Removed it.
  • Remove usages of tracker.mutex outside of the tracker object, ensures usage is more obviously correct
  • Fix race condition that occurred when generating the tracker string
  • tracker.Value is only reading, so use RLock

I ran the demo with the race detector several times and wasn't able to duplicate the race found in https://github.com/jedib0t/go-pretty/pull/153/checks?check_run_id=2266140256, but it seems pretty obvious that this is the culprit.

@virtuald
Copy link
Contributor Author

virtuald commented Apr 5, 2021

Worth noting that the only reason tests are failing is because of coveralls doesn't seem to be configured properly.

@jedib0t
Copy link
Owner

jedib0t commented Apr 6, 2021

Thanks for the change. Will look into it this week. I've to figure out a way forward for coveralls with GitHub Actions.

@jedib0t
Copy link
Owner

jedib0t commented Apr 6, 2021

@virtuald The changes look okay to me. Can you rebase to latest on the master branch?

- tracker.mutexPrv is useless since it only protects writes. Removed it.
- Remove usages of tracker.mutex outside of the tracker object, ensures usage is more obviously correct
- Fix race condition that occurred when generating the tracker string
@virtuald
Copy link
Contributor Author

virtuald commented Apr 7, 2021

Rebased.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c91811f on virtuald:races into 939f457 on jedib0t:master.

@jedib0t jedib0t merged commit 4ce9954 into jedib0t:master Apr 7, 2021
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