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

Add RW Lock to allComponentSchemas to Prevent Data Race Error #344

Merged
merged 5 commits into from
Oct 29, 2024

Conversation

califlower
Copy link
Contributor

  • solves issue Data Race using GetAllComponentSchemas #333
  • Accessing GetAllComponentSchemas causes a race condition error during usage.
  • Adds a RWLock to ensure that the map object is safely created across go routines. This pattern is used for other similar fields

@califlower califlower changed the title Add RW Lock to allComponentSchemas to Prevent Add RW Lock to allComponentSchemas to Prevent Data Race Error Oct 24, 2024
Copy link

codecov bot commented Oct 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.61%. Comparing base (5ac8657) to head (5c3c79d).
Report is 7 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #344      +/-   ##
==========================================
- Coverage   99.62%   99.61%   -0.01%     
==========================================
  Files         165      165              
  Lines       21050    21064      +14     
==========================================
+ Hits        20971    20983      +12     
- Misses         74       76       +2     
  Partials        5        5              
Flag Coverage Δ
unittests 99.61% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@califlower
Copy link
Contributor Author

I can add a test here, just wasn't sure about testing the race condition it was kind of difficult

@califlower
Copy link
Contributor Author

I added a concurrent access test and a nil test. That should keep the coverage where it was at before

@califlower
Copy link
Contributor Author

RIP. Doesn't seem like codecov is correct :<

@daveshanley
Copy link
Member

daveshanley commented Oct 29, 2024

The codecov is correct, I checked out your branch and this is what happens after running all tests.

Screenshot 2024-10-29 at 10 30 30 AM

The problem is the test.

func TestSpecIndex_GetAllComponentSchemas_NilIndex(t *testing.T) {
	index := &SpecIndex{}
	schemas := index.GetAllComponentSchemas()
	assert.Nil(t, schemas, "Expected GetAllComponentSchemas to return nil when index is nil")
}

index cannot be nil here, it will never be nil. To fix it:

func TestSpecIndex_GetAllComponentSchemas_NilIndex(t *testing.T) {
	var index *SpecIndex
	schemas := index.GetAllComponentSchemas()
	assert.Nil(t, schemas, "Expected GetAllComponentSchemas to return nil when index is nil")
}

Copy link
Member

@daveshanley daveshanley left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution.

@daveshanley daveshanley merged commit 6cb2bf8 into pb33f:main Oct 29, 2024
3 of 4 checks passed
@califlower
Copy link
Contributor Author

Ah my bad. Thank you for fixing it and merging! Really appreciate it!

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.

2 participants