Skip to content
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

WIP: Stop leaking goroutines in newMultiWatch #50

Closed
wants to merge 1 commit into from

Conversation

dgn
Copy link
Contributor

@dgn dgn commented Sep 3, 2019

This fixes a goroutine leak in cases where Watch() is called again without a new List() inbetween Watch() calls. This happens when one of the underlying watches returns a ConnectionRefused error. See https://github.com/kubernetes/client-go/blob/3fe2abece89efc7b3a2da0ff549525f20f435263/tools/cache/reflector.go#L286

Note that I stumbled upon this leak while fixing another bug; this probably has other implications, so I'll mark it WIP for now.

@luksa
Copy link
Contributor

luksa commented Sep 3, 2019

This happens when one of the underlying watches returns a ConnectionRefused error.

Note that watches are also closed by the API Server every few minutes. That's another case where Watch() is called again without a new List().

pkg/listwatch/listwatch.go Outdated Show resolved Hide resolved
@dgn dgn force-pushed the stop-leaking branch 2 times, most recently from 9ecbdd3 to 266a664 Compare September 3, 2019 12:03
@dgn
Copy link
Contributor Author

dgn commented Sep 3, 2019

This happens when one of the underlying watches returns a ConnectionRefused error.

Note that watches are also closed by the API Server every few minutes. That's another case where Watch() is called again without a new List().

That's correct, but the leak is only triggered when Watch() fails and is not followed by either a call to Stop() or List(). The reflector will in fact call Stop() before calling Watch() again in that case.

This fixes a goroutine leak in cases where Watch() is called again
without a new List() inbetween Watch() calls. This happens when one
of the underlying watches returns a ConnectionRefused error. See
https://github.com/kubernetes/client-go/blob/3fe2abece89efc7b3a2da0ff549525f20f435263/tools/cache/reflector.go#L286
@dgn dgn changed the base branch from maistra-1.0 to maistra-1.1 January 20, 2020 12:59
dgn pushed a commit to dgn/istio-maistra that referenced this pull request Jan 28, 2020
* don't use mtls for bookinfo destination rules

Co-authored-by: Philipp Stehle <philipp.stehle@sap.com>
Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>

* Remove old webhook when switching controlplanes

Co-authored-by: Philipp Stehle <philipp.stehle@sap.com>
Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>

* Switch control planes after update

Co-authored-by: Philipp Stehle <philipp.stehle@sap.com>
Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>

* Add owner reference to webhook

Co-authored-by: Philipp Stehle <philipp.stehle@sap.com>
Signed-off-by: Ralf Pannemans <ralf.pannemans@sap.com>
dgn pushed a commit to dgn/istio-maistra that referenced this pull request Jan 28, 2020
This initial take on the prow infrastructure is targeted at
linting to begin with.
@dgn dgn closed this May 11, 2020
@dgn dgn deleted the stop-leaking branch January 23, 2023 22:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants