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

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

Merged
merged 4 commits into from
Nov 11, 2022
Merged

Conversation

fortytw2
Copy link
Contributor

@fortytw2 fortytw2 commented Nov 9, 2022

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.

@fortytw2 fortytw2 changed the title add missing s.mutex usage fix race condition in (*Spinner).Print Nov 9, 2022
@chelnak
Copy link
Owner

chelnak commented Nov 10, 2022

@fortytw2 Thanks so much for this!

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

@fortytw2
Copy link
Contributor Author

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

@chelnak chelnak self-assigned this Nov 10, 2022
@chelnak chelnak added the bug Something isn't working label Nov 10, 2022
@chelnak
Copy link
Owner

chelnak commented Nov 10, 2022

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.

manager.go Outdated Show resolved Hide resolved
manager.go Outdated Show resolved Hide resolved
manager.go Show resolved Hide resolved
@chelnak
Copy link
Owner

chelnak commented Nov 11, 2022

@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
Copy link
Contributor Author

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

@chelnak
Copy link
Owner

chelnak commented Nov 11, 2022

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

Copy link
Owner

@chelnak chelnak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect 🤩

@chelnak chelnak merged commit 29502cc into chelnak:main Nov 11, 2022
@chelnak
Copy link
Owner

chelnak commented Nov 11, 2022

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

@fortytw2
Copy link
Contributor Author

Thanks @chelnak !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants