-
Notifications
You must be signed in to change notification settings - Fork 591
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(manager) lock client creation #2070
Conversation
906fc43
to
13b950b
Compare
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.
This patch does seem to fix the immediate issue, however having the calling code being responsible for managing the locks for the underlying race condition makes maintenance and future contributions a bit more difficult. The burden should be on the implementation of the functionality, I suggest:
- move the locking mechanism into the
GetKongClient()
method (OR the lowest level needed to avoid the collision) - make the mutex a part of the
Config
rather than a glob if the lock is moved into aConfig
method - whatever method or function the lock ends up moving to add godoc documentation to it to help provide some additional context
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.
We can get away without locks altogether if we follow the dependency inversion pattern and instantiate the client only once, and pass it down to the callers (e.g. as a function argument) for reuse.
Having a method called |
13b950b
to
779c368
Compare
Add a mutex and lock it when creating Kong admin clients. Attempting to create multiple workspaced clients simultaneously can result in a race condition where both clients check for the workspace before either creates it, both clients will attempt to create the workspace, and the second client to attempt creation will hit a unique violation.
Went with the lock within the function. We don't currently create meaningfully different clients, but potentially could if we start interacting with separate Kong instances and/or workspaces from a single controller, which we've discussed in the past. |
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.
One suggestion and then LGTM
Co-authored-by: Shane Utt <[email protected]>
What this PR does / why we need it:
Add a mutex and lock it when creating Kong admin clients. Attempting to
create multiple workspaced clients simultaneously can result in a race
condition where both clients check for the workspace before either
creates it, both clients will attempt to create the workspace, and the
second client to attempt creation will hit a unique violation.
Which issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes CI errors like:Special notes for your reviewer:
PR Readiness Checklist:
Complete these before marking the PR as
ready to review
:CHANGELOG.md
release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR