librespot-golang
librespot-golang copied to clipboard
librespot-org
Add AudioFile cancellation
This allows an AudioFile to be cancelled. This is useful to avoid network traffic and memory consumption when you wish to switch tracks before the first has finished.
Running a client through the race detector with this patch I found that this creates a data race on Cancel(). The simple fix is to just protect the cancelled variable with a (RW)Mutex, but the more idiomatic way is probably to use context and cancellation.
@anisse I don't really understand what the race condition would be, can you elaborate?
I wish I had saved the race detector output (will try to reproduce), but the gist is, from memory:
- AudioFile fetching happens in a goroutine
- that goroutine regularly reads the cancelled variable to know when to stop
- you call Cancel () from another goroutine, so there's a datarace when writing the cancelled variable while the other goroutine might read it.
I fixed it like this in my fork : https://github.com/anisse/librespot-golang/commit/23aeb876a4a852604560a67a87f109168a65e38e and the parent commit : https://github.com/anisse/librespot-golang/commit/62ca6aa0bcd343ea2d34463d62895dfbca705690
Reflecting again, atomics could have been used instead of a mutex.
I can see the chance for a read-whilst-write, but I'm not sure that it's problematic anyway, unless Golang forces that to be an error? If we incorrectly read that it's not-cancelled then it will be cancelled the next time we check, very shortly after, we cannot incorrectly read that it is cancelled as far as I can tell.
Have I missed something?
Golang doesn't force that to be an error per se, but your CPU might serve you incoherent data. The Go doc specifies that concurrent write and read access must be serialized. See: https://golang.org/ref/mem
Programs that modify data being simultaneously accessed by multiple goroutines must serialize such access
And the guide to the data races : https://golang.org/doc/articles/race_detector.html
@anisse Could you make a PR with your patch so I can integrate it here for everyone? Thanks
@xplodwild : done with #16 , but I didn't include the fix to @benpye 's cancellation since it hasn't been merged yet.
@anisse Thanks, I had notifications off for the repo so I missed a bunch of things, but will review them throughout the day and get up to speed.
Hey @xplodwild @anisse , I'll try and update this PR tonight to fix the data race.