-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
🐛 Prevent race when informers are started more than once #2758
Conversation
Done |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman 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 |
/hold |
pkg/cache/internal/informers.go
Outdated
@@ -186,6 +187,12 @@ type Informers struct { | |||
// Start calls Run on each of the informers and sets started to true. Blocks on the context. | |||
// It doesn't return start because it can't return an error, and it's not a runnable directly. | |||
func (ip *Informers) Start(ctx context.Context) error { | |||
select { | |||
case <-ip.startWait: |
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.
Do we need a lock around this?
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.
No, this is a channel
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.
That's not how I meant it :)
What if you have one goroutine in l.198 and another one then just going through l.191?
I would assume the same data race.
Don't we have to make sure an additional goroutine can't go through l.190-l.195 while another one is inside
l.197-216?
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.
Put all of it under the lock
/hold cancel |
b9c9be8
to
c2c96b0
Compare
If `Informers` are started a second time, there is a possibility for a data race because it sets a `ctx` field on itself. This write is protected by a mutex, but reads from that field are not.
/lgtm /hold |
LGTM label has been added. Git tree hash: 59bffd29e2eca09280185b4878884fe155112efb
|
/hold cancel |
If
Informers
are started a second time, there is a possibility for a data race because it sets actx
field on itself. This write is protected by a mutex, but reads from that field are not.As it is generally unclear and untested what happens when a cache is started a second time, simply error out to make the user aware that they were starting the cache a second time.