-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Avoid possible race condition using locks #6850
Conversation
35ab4b2
to
58bea07
Compare
plugins/bundle/plugin.go
Outdated
@@ -157,7 +157,10 @@ func (p *Plugin) Reconfigure(ctx context.Context, config interface{}) { | |||
// Look for any bundles that have had their config changed, are new, or have been removed | |||
newConfig := config.(*Config) | |||
newBundles, updatedBundles, deletedBundles := p.configDelta(newConfig) | |||
|
|||
p.mtx.Lock() |
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.
There already is a p.cfgMtx
that is held for this purpose further above
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.
Yes, thanks for pointing out.
1a67ff8
to
c3689e8
Compare
57f8f05
to
5c620d6
Compare
0603dc8
to
291775a
Compare
@@ -62,7 +62,7 @@ type Plugin struct { | |||
downloaders map[string]Loader | |||
logger logging.Logger | |||
mtx sync.Mutex | |||
cfgMtx sync.Mutex | |||
cfgMtx sync.RWMutex |
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 need to hold the write lock while updating the config. Also if you could look at the test failures that would be great. Thanks.
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. I have addressed it.
Could you please have a look?
ae231b0
to
8d9ff08
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.
@Pushpalanka the changes seem ok. There are places where previously atomic operations are no longer atomic. So we need to be careful about the impact of the change.
plugins/bundle/plugin.go
Outdated
p.cfgMtx.Lock() | ||
p.config = *parsedConfig | ||
p.cfgMtx.Unlock() | ||
|
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.
Why do need to hold the lock during plugin instantiation?
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 for raising this! Was carried away by the line 74 parsedConfig.Bundles
as it reads the Bundles from the config. But yes we don't need this at initiation.
plugins/bundle/plugin.go
Outdated
p.cfgMtx.RLock() | ||
bundles := p.config.Bundles | ||
p.cfgMtx.RUnlock() | ||
|
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.
Currently the below operation is performed atomically. Do we know the consequences of changing that behavior?
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 needs to be changed (be atomic for the iteration).
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
fc89516
to
927bf03
Compare
plugins/bundle/plugin.go
Outdated
@@ -310,10 +313,14 @@ func (p *Plugin) UnregisterBulkListener(name interface{}) { | |||
|
|||
// Config returns the plugins current configuration | |||
func (p *Plugin) Config() *Config { | |||
p.cfgMtx.RLock() | |||
defer p.cfgMtx.RUnlock() |
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 aren't making a copy of the config, so what benefit is the lock giving us here?
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 am thinking we still need to protect against reading while writes are in-progress.
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.
Just understood the point. Will correct.
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
Signed-off-by: Pushpalanka Jayawardhana <pushpalanka.jayawardhana@zalando.de>
e28d88c
to
9b945de
Compare
|
||
// Look for any bundles that have had their config changed, are new, or have been removed | ||
newConfig := config.(*Config) | ||
newBundles, updatedBundles, deletedBundles := p.configDelta(newConfig) | ||
p.config = *newConfig | ||
p.cfgMtx.Unlock() |
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.
Have you considered the scenario of concurrent calls to Reconfigure
? Could it break the following code flow?
What if we keep the existing code here?
p.cfgMtx.RLock() | ||
src := p.config.Bundles[name] | ||
p.cfgMtx.RUnlock() | ||
|
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.
If the write lock on the config was acquired at this point, won't it affect persistBundle
?
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 couldn't see a concern with persistBundle
as src is read to a new variable and passed. However if we acquire the write lock for the whole method of Reconfigure
as in previous comment, then this read lock will be a problem.
Not sure if I understood your concern here.
@Pushpalanka thanks for the back and forth on this and also for addressing my comments. Dealing with mutexes is tricky and sometimes it's hard to tell if a change can have unintended consequences. So I appreciate your work on this. |
Thanks @ashutosh-narkar for your patience and support so far. I am surfing unknown waters here, yet understand this is a tricky and critical code segments I am touching. Feel free to take over as well if this becoming less efficient. :) |
I'll spend sometime looking into this. Thanks for reporting the issue and working on a fix. |
Closing in favor of #6921. Thanks for working on this @Pushpalanka! |
Thanks a lot @ashutosh-narkar 👍 |
Why the changes in this PR are needed?
to address #6849
What are the changes in this PR?
Add locks when reading and writing the bundle plugin configuration.
No tests are added, as not sure exactly on how to write a test for this. The issue is experienced when checking for race conditions while using bundle plugin status listeners while using OPA as a library.