Skip to content

fix: Avoid data races for cacheEntries and index files#556

Merged
cbuto merged 5 commits into
helm:mainfrom
cbuto:fix/cache-race
Apr 8, 2022
Merged

fix: Avoid data races for cacheEntries and index files#556
cbuto merged 5 commits into
helm:mainfrom
cbuto:fix/cache-race

Conversation

@cbuto

@cbuto cbuto commented Mar 4, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

Closes #546.

This should resolve the data races seen when running the test suite with the -race flag.

I took some of the work/feedback from #549 (thanks @nerdeveloper!) and added some additional changes to ensure we are locking when access the index file and cache entries.

I still would like to spend more time on the performance aspect to see if adjusting the locks could help.

Overall this change as is has a slight negative effect on median response time and a extremely positive effect the p95 response time. I assume the p95 response without these changes is fairly high due to data races between requests.

Here are the results from the very small load tests I ran locally.

  1. Start chartmuseum locally
rm -rf /tmp/chartmuseum-loadtest-*  
make build-mac && ./bin/darwin/amd64/chartmuseum --storage local --storage-local-rootdir /tmp
  1. Run locust with 10 users/10 spawn rate

Here are the test run branches:

Test 8 = main
Test 9 = fix/cache-race
Test 10 = main
Test 11 = fix/cache-race

Screen Shot 2022-03-04 at 6 12 57 PM

@cbuto cbuto self-assigned this Mar 4, 2022
@cbuto
cbuto marked this pull request as ready for review March 6, 2022 10:52
@cbuto
cbuto requested review from jdolitsky and scbizu March 6, 2022 10:52
@cbuto cbuto added this to the v0.15.0 milestone Mar 6, 2022
@scbizu

scbizu commented Mar 7, 2022

Copy link
Copy Markdown
Contributor

wow , looks good .

I will spend some more time to read over this PR 👀

However , it will conflict with #549 ?

@cbuto

cbuto commented Mar 7, 2022

Copy link
Copy Markdown
Contributor Author

wow , looks good .

I will spend some more time to read over this PR 👀

However , it will conflict with #549 ?

Thanks! No rush at all. But yeah builds off of #549 and the initial work @nerdeveloper did.

@scbizu

scbizu commented Mar 7, 2022

Copy link
Copy Markdown
Contributor

I read some of this code , and have a question: what we do is actually make the rough-grain lock (RegenerationLock) to be the fine-grain lock (RepoLock & IndexLock) ? But do we need to keep these rough-grain lock after we did this ?

And I see the response time should be reduced literally , and this optimization also related to the #396 .

@cbuto

cbuto commented Mar 8, 2022

Copy link
Copy Markdown
Contributor Author

I read some of this code , and have a question: what we do is actually make the rough-grain lock (RegenerationLock) to be the fine-grain lock (RepoLock & IndexLock) ? But do we need to keep these rough-grain lock after we did this ?

And I see the response time should be reduced literally , and this optimization also related to the #396 .

good call! I think you are right, I removed the RegenerationLock in 48b75c7 and it looks good. Some quick load test results showed no change but i don't think that lock was necessary anymore!

@scbizu

scbizu commented Mar 8, 2022

Copy link
Copy Markdown
Contributor

Looks much more comfortable now , I will review these all changes and make some tests on my mac this weekend .

scbizu
scbizu requested changes Mar 11, 2022
View reviewed changes

@scbizu scbizu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The code here should be really careful for both writing and reviewing XD

Comment thread pkg/chartmuseum/server/multitenant/cache.go Outdated
Comment thread pkg/chartmuseum/server/multitenant/server_test.go Outdated
Comment thread pkg/chartmuseum/server/multitenant/server_test.go Outdated
@scbizu

scbizu commented Mar 11, 2022

Copy link
Copy Markdown
Contributor

@jdolitsky hihi , Could you also join the reviewing , I think it is an important fix or refactor PR .

scbizu
scbizu approved these changes Mar 12, 2022
View reviewed changes

@scbizu scbizu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@cbuto
cbuto requested a review from nerdeveloper March 12, 2022 18:04
@nerdeveloper

nerdeveloper commented Mar 14, 2022

Copy link
Copy Markdown
Member

Awesome work there @cbuto

@cbuto cbuto mentioned this pull request Mar 22, 2022
Closed
@cbuto

cbuto commented Apr 7, 2022

Copy link
Copy Markdown
Contributor Author

after rebasing with #551, some new data races showed up while running the tests (it seems to be flakey)

jdolitsky
jdolitsky approved these changes Apr 8, 2022
View reviewed changes

@jdolitsky jdolitsky left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

fantastic. thanks for addressing this (apologies for delay in review!)

@cbuto

cbuto commented Apr 8, 2022

Copy link
Copy Markdown
Contributor Author

latest race fixed in 3946435, I ran the tests quite a few times just to be sure and it looks good!

@cbuto
cbuto merged commit bc596c0 into helm:main Apr 8, 2022
This was referenced Apr 9, 2022
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jdolitsky jdolitsky jdolitsky approved these changes
@scbizu scbizu scbizu approved these changes
@nerdeveloper nerdeveloper nerdeveloper approved these changes

Assignees

@cbuto cbuto

Labels

size/XL

Projects

None yet

Milestone

v0.15.0

Development

Successfully merging this pull request may close these issues.

Fix race conditions and add -race flag to test.sh

4 participants

@cbuto @scbizu @nerdeveloper @jdolitsky