search

gosimple / slug

URL-friendly slugify with multiple languages support.
Mozilla Public License 2.0
1.15k stars 108 forks source link

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

Open timbunce opened 4 years ago

timbunce commented 4 years ago

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

matrixik commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

Hello @matrixik. Anything else needed for this PR?