-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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 concurrent usage issue of the same scheme #107876
Fix concurrent usage issue of the same scheme #107876
Conversation
871b130
to
339cb03
Compare
/retest |
1 similar comment
/retest |
var mu = sync.Mutex{} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it'd be better to add a mutex as a member of the Scheme
struct and use that internally in AddKnownTypeWithName
when needed 🤔
I feel like that'd be cleaner than introducing a global lock and passing it to the constructor and initializing it as part of init
. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it'd be better to add a mutex as a member of the
Scheme
struct and use that internally inAddKnownTypeWithName
when needed
That approach sounds good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if it'd be better to add a mutex as a member of the Scheme struct and use that internally in AddKnownTypeWithName when needed
That approach sounds good
Actually, I'm now remembering what past me was thinking here. Because the scheme is used for multi-threaded reads, it would take a fair amount of work to wrap everything. If the tests were updated instead to have a newTestScheme
function that returned a unique scheme for each test (the tests probably didn't intended to mutate a global scheme anyway), that will fix the issue without the fanout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Madhav and David.
Function NewTestScheme
has been added to return a unique scheme for each test. PTAL
/assign @MadhavJivrajani |
339cb03
to
74d412a
Compare
74d412a
to
d66b3ed
Compare
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jlsong01!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, jlsong01, MadhavJivrajani The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Because of this scheme mutation, this leads to concurrent use of the same scheme to cause the program to panic. It would be ideal to do this in some concurrent safe manner.
Which issue(s) this PR fixes:
Fixes #107823, #107874
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: