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.

Sign up for GitHub

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

[13.4-stable] pillar/devicenetwork: fix goroutine leak #4425

Merged
merged 2 commits into from
Nov 6, 2024
Merged

[13.4-stable] pillar/devicenetwork: fix goroutine leak #4425

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
24d45ca
pillar/devicenetwork: fix goroutine leak
christoph-zededa Oct 31, 2024
02a85ed
pillar/devicenetwork: goroutine leak test
christoph-zededa Oct 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 23 additions & 10 deletions pkg/pillar/devicenetwork/dns.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ func ResolveWithPortsLambda(domain string,

quit := make(chan struct{})
work := make(chan struct{}, DNSMaxParallelRequests)
resolvedIPsChan := make(chan []DNSResponse)
defer close(work)

resolvedIPsChan := make(chan []DNSResponse, 1)
defer close(resolvedIPsChan)

countDNSRequests := 0
var errs []error
var errsMutex sync.Mutex
Expand All @@ -131,10 +135,13 @@ func ResolveWithPortsLambda(domain string,
copy(srcIPCopy, srcIP)
countDNSRequests++
go func(dnsIP, srcIP net.IP) {
select {
case work <- struct{}{}:
// if writable, means less than dnsMaxParallelRequests goroutines are currently running
}
defer func() {
wg.Done()
<-work
}()
// if writable, means less than dnsMaxParallelRequests goroutines are currently running
work <- struct{}{}

select {
case <-quit:
// will return in case the quit chan has been closed,
Expand All @@ -149,22 +156,28 @@ func ResolveWithPortsLambda(domain string,
defer errsMutex.Unlock()
errs = append(errs, err)
}
if response != nil {
resolvedIPsChan <- response
if response != nil && len(response) > 0 {
select {
case resolvedIPsChan <- response:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I didn't check your latest changes before they were merged, but this will panic if resolvedIPsChan is already closed.
https://go.dev/play/p/I96AZ_2-kcf
It was better when select was used between this channel and reading quit, while resolvedIPsChan was left for GC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thx, I will change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😨😨😨

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@milan-zededa go back to use quit or go back and let the GC close resolvedIPsChan?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need both but maybe you have simpler solution in mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually @OhmSpectator and I discussed and it seems, no change is necessary, because resolvedIPsChan will only be closed after

	defer func() {
		close(quit)
		<-wgChan
	}()

which will only finish after

	go func() {
		wg.Wait()
		close(wgChan)
	}()

which will only finish after all goroutines of the waitgroup finished and that means, that no goroutine can write into resolvedIPsChan

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, true, I missed this.
This function is not for the faint-hearthed :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No risk, no fun :-)

default:
}
}
<-work
wg.Done()
}(dnsIPCopy, srcIPCopy)
}
}
}

wgChan := make(chan struct{})

go func() {
wg.Wait()
close(wgChan)
}()

defer func() {
close(quit)
<-wgChan
}()
select {
case <-wgChan:
var responses []DNSResponse
Expand All @@ -183,7 +196,7 @@ func ResolveWithPortsLambda(domain string,
}
return responses, errs
case ip := <-resolvedIPsChan:
close(quit)
return ip, nil
}

}
3 changes: 2 additions & 1 deletion pkg/pillar/devicenetwork/dns_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/lf-edge/eve/pkg/pillar/devicenetwork"
"github.com/lf-edge/eve/pkg/pillar/netmonitor"
"github.com/lf-edge/eve/pkg/pillar/types"
"go.uber.org/goleak"
)

func createNetmonitorMockInterface() []netmonitor.MockInterface {
Expand Down Expand Up @@ -161,7 +162,7 @@ func TestDnsResolveTimeout(t *testing.T) {
}

func TestResolveWithPortsLambda(t *testing.T) {
t.Parallel()
defer goleak.VerifyNone(t, goleak.IgnoreAnyFunction("go.opencensus.io/stats/view.(*worker).start"), goleak.IgnoreCurrent())

expectedIP := net.IP{1, 2, 3, 4}

Expand Down
1 change: 1 addition & 0 deletions pkg/pillar/go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,7 @@ require (
github.com/pborman/uuid v1.2.1 // indirect
github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.68.0 // indirect
github.com/spf13/pflag v1.0.5 // indirect
go.uber.org/goleak v1.3.0 // indirect
k8s.io/apiextensions-apiserver v0.28.1 // indirect
k8s.io/klog/v2 v2.110.1 // indirect
k8s.io/kube-openapi v0.0.0-20231010175941-2dd684a91f00 // indirect
Expand Down
2 changes: 2 additions & 0 deletions pkg/pillar/go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1997,6 +1997,8 @@ go.uber.org/automaxprocs v1.5.1/go.mod h1:BF4eumQw0P9GtnuxxovUd06vwm1o18oMzFtK66
go.uber.org/goleak v1.1.10/go.mod h1:8a7PlsEVH3e/a/GLqe5IIrQx6GzcnRmZEufDUTk4A7A=
go.uber.org/goleak v1.1.12/go.mod h1:cwTWslyiVhfpKIDGSZEM2HlOvcqm+tG4zioyIeLoqMQ=
go.uber.org/goleak v1.2.1/go.mod h1:qlT2yGI9QafXHhZZLxlSuNsMw3FFLxBr+tBRlmO1xH4=
go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
go.uber.org/multierr v1.1.0/go.mod h1:wR5kodmAFQ0UK8QlbwjlSNy0Z68gJhDJUG5sjR94q/0=
go.uber.org/multierr v1.6.0/go.mod h1:cdWPpRnG4AhwMwsgIHip0KRBQjJy5kYEpYjJxpXp9iU=
go.uber.org/zap v1.10.0/go.mod h1:vwi/ZaCAaUcBkycHslxD9B2zi4UTXhF60s6SWpuDF0Q=
Expand Down
5 changes: 5 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/.gitignore
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/.golangci.yml
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

74 changes: 74 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/CHANGELOG.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

21 changes: 21 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/LICENSE
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 45 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/Makefile
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

74 changes: 74 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/README.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

22 changes: 22 additions & 0 deletions pkg/pillar/vendor/go.uber.org/goleak/doc.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading