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

ISSUE#195 github.com/spf13/viper is not thread safe #246

Merged
merged 4 commits into from
May 28, 2021
Merged

Conversation

platsko
Copy link
Contributor

@platsko platsko commented May 6, 2021

There is a suggestion to solve the problem with the Viper package using wrapper functions. Let's discuss this.

@platsko platsko requested review from Sriep and MurashovVen May 6, 2021 15:30
@platsko platsko linked an issue May 6, 2021 that may be closed by this pull request
@platsko platsko marked this pull request as draft May 7, 2021 08:38
@platsko platsko force-pushed the issues/195 branch 3 times, most recently from fb3ea8f to c858b19 Compare May 13, 2021 13:47
@platsko platsko marked this pull request as ready for review May 13, 2021 13:49
@platsko platsko changed the title [WIP] ISSUE#195 github.com/spf13/viper is not thread safe ISSUE#195 github.com/spf13/viper is not thread safe May 13, 2021
Sriep
Sriep previously requested changes May 13, 2021
Copy link
Contributor

@Sriep Sriep left a comment

Choose a reason for hiding this comment

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

Looks promising. We need unit tests for core/viper. What about reading in various 0chain config files, writing them out then rereading them? Then checking it worked.

We also want to test concurrency, so maybe run several tests in parallel that modify the viper store, in a similar way to 0chain, and confirm it can cope.

@platsko
Copy link
Contributor Author

platsko commented May 15, 2021

@Sriep
During testing, significant shortcomings in the implementation were revealed, many methods cannot be thread-safe - I think it would be better to get rid of them altogether. Please see the last commit.

@platsko platsko marked this pull request as draft May 15, 2021 09:00
@Sriep
Copy link
Contributor

Sriep commented May 17, 2021

Do we use any of the non-thread-safe methods? Yes AddConfigPath and SetConfigType.

Eye on the goal: We don't want race errors. So either avoid using unsafe viper methods or make sure they are only accessed from one thread.

@Sriep
Copy link
Contributor

Sriep commented May 20, 2021

I think we just have to let the non-thread-safe methods stand. Assume they are always called safely.

We can then raise a separate issue to check our usage of non-thread-safe viper methods and, if necessary, fix that in a separate PR.

@platsko platsko marked this pull request as ready for review May 24, 2021 13:59
- fix error core/viper/viper.go:261: undefined: os.ReadFile note: module requires Go 1.15
@Sriep Sriep merged commit e547ff0 into master May 28, 2021
@Sriep Sriep deleted the issues/195 branch May 28, 2021 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

github.com/spf13/viper is not thread safe
2 participants