-
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
🐛 Controller.Watch() should not store watches if already started #1163
🐛 Controller.Watch() should not store watches if already started #1163
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
780fdfb
to
cf12923
Compare
The controller internal struct holds a list of watches (as []watchDescription) when someone calls .Watch() to then start the watches and informers once we're ready to call Start(). This behavior caused a memory leak in the case Watch was called after a controller has already been started and if the source.Kind's cache was either stopped or not available any longer. The leak was caused by the watches internal slice holding on to all references to each watch ever issued (and their respective caches). Signed-off-by: Vince Prignano <vincepri@vmware.com>
cf12923
to
a6ec31f
Compare
LGTM |
@vincepri are you sure this properly solves the problem? Even when the Controller doesn't hold the Source anymore, the underlying informer will have an eventhandler attached if was ever in use, preventing it from being garbage collected - or am I missing something? /lgtm |
@alvaroaleman we are dynamically creating a Cache and closing its stop channel when we're done with it, which GC's the informers. |
What Andy said, if the stop channel is called, the informer properly stops, although the controller holds a reference to the KindSource which has a reference to the underlying cache, which will never be garbage collected |
okay, thanks for clarifying |
The controller internal struct holds a list of watches
(as []watchDescription) when someone calls .Watch() to then start the
watches and informers once we're ready to call Start().
This behavior caused a memory leak in the case Watch was called after
a controller has already been started and if the source.Kind's cache was
either stopped or not available any longer. The leak was caused by the
watches internal slice holding on to all references to each watch ever
issued (and their respective caches).
Signed-off-by: Vince Prignano vincepri@vmware.com
/assign @alvaroaleman @DirectXMan12 @pwittrock
I'll backport the fix to release-0.5 and release-0.6 if we all agree that this is the fix we want. For consistency, I also nil'd the c.watches slice after Start() has been called and the controller has started.