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

Make safe for use with concurrent goroutines (fix race hazards) #41

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

timbunce
Copy link

Also removes the forcing of go version 1.13 since it's not needed.

slug.go Show resolved Hide resolved
slug.go Show resolved Hide resolved
slug.go Show resolved Hide resolved
Also removes the forcing of go version 1.13 since it's not needed.
Copy link
Member

@matrixik matrixik left a 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.

@matrixik
Copy link
Member

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.

@matrixik
Copy link
Member

Travis run tests by default with -race https://github.com/gosimple/slug/blob/master/.travis.yml#L18

So if you add test it should find if anything bad happen to the code at future.

@timbunce
Copy link
Author

timbunce commented Nov 3, 2019

Please, add test that will catch race condition if some change break this code.
Travis run tests by default with -race

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 go test -race -p N ... to change the level of parallelism.

@timbunce
Copy link
Author

timbunce commented Nov 3, 2019

Also, re the go 1.13 change, I did that wrong. I should have changed it to go 1.12 or go 1.11 rather than removing it.

@timbunce
Copy link
Author

timbunce commented Jan 2, 2020

Hello @matrixik. Anything else needed for this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants