-
Notifications
You must be signed in to change notification settings - Fork 108
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
Make safe for use with concurrent goroutines (fix race hazards) #41
base: master
Are you sure you want to change the base?
Conversation
Also removes the forcing of go version 1.13 since it's not needed.
82a8e7a
to
18f9540
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, add test that will catch race condition if some change break this code.
Code itself looks good.
Hello and thank for this changes. I moved removing of Go 1.13 requirement to another PR and merged with master #42 because I always squash pull requests and wanted it in separate commit. Thank you for showing me this. |
Travis run tests by default with So if you add test it should find if anything bad happen to the code at future. |
The -race flag only detects races encountered by the test code that's run. The way the current tests are written doesn't trigger races. The original API for the module is built around modifying globals, so naturally there is, and always will be, a race hazard using that API. That's why I've added a new API. The old API is now implemented in terms of the new one, so testing the old is also testing the new. In 05c89ad I've added t.Run and t.Parallel calls to demonstrate the problem. I've left the t.Parallel calls commented out. If they are uncommented then the tests with fail. Use |
Also, re the |
Hello @matrixik. Anything else needed for this PR? |
Also removes the forcing of go version 1.13 since it's not needed.