search

chelnak / ysmrr

YSMRR is a package that provides simple multi-line compatible spinners for Go applications.
MIT License
71 stars 6 forks source link

fix race condition in `(*Spinner).Print` #46

Closed fortytw2 closed 1 year ago

fortytw2 commented 1 year ago

Hey! Started using this repo after needing multiple spinners (migrated from yacspin) and noticed a race condition caused by (*Spinner).Print( when UpdateMessage( is used from different goroutines.

I can add a test that reproduces this race detector failure if you'd like, too.

chelnak commented 1 year ago

@fortytw2 Thanks so much for this!

If you wouldn't mind adding a test that would be perfect!

fortytw2 commented 1 year ago

Of course! Added a test that fails without the additional locks @chelnak. Thanks for the great library :)

chelnak commented 1 year ago

Thank you for the test.

It's actually highlighted something unexpected. The race condition actually appears to be coming from AddSpinner.

This doesn't happen when you use a mutex inside the method and seems to be validated by your test.

Do you see a similar thing?

I can see you spotted it too.

chelnak commented 1 year ago

@fortytw2 If there is anything I can do to help progress this let me know.

As soon as we merge, I'm happy to cut a release 🙂

fortytw2 commented 1 year ago

Should be good to go @chelnak - let me know if you'd like me to rebase/squash commits in here before merging.

chelnak commented 1 year ago

Nah I think we are all good. Your commits are clear enough to me!

chelnak commented 1 year ago

@fortytw2 https://github.com/chelnak/ysmrr/releases/tag/v0.2.1 ❤️

fortytw2 commented 1 year ago

Thanks @chelnak !