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

Install:RunWithContext function have potential goroutine leak #10489

Closed
MaxCaresYww opened this issue Dec 21, 2021 · 3 comments
Closed

Install:RunWithContext function have potential goroutine leak #10489

MaxCaresYww opened this issue Dec 21, 2021 · 3 comments
Labels
bug Categorizes issue or PR as related to a bug. Stale

Comments

@MaxCaresYww
Copy link

Output of helm version:
3.7.0

Output of kubectl version:
I use Helm not from command line but take it as a go package and integrate it into my go based code.

Cloud Provider/Platform (AKS, GKE, Minikube etc.):
k3s.

In "RunWithContext" method of "*Install" structure, there is following code snippet:

 // https://github.com/helm/helm/blob/v3.7.2/pkg/action/install.go
rChan := make(chan resultMessage)
go i.performInstall(rChan, rel, toBeAdopted, resources)
go i.handleContext(ctx, rChan, rel)
result := <-rChan

This is probably problematic as the length of this channel is 1 and there are two places will send resultMessage to this channel. One of them will success, and the other one will block there. This won't be seem if user is using helm from command line as once that command is exited, all resources are freed including the goroutine. But when this is used by other application (in my case), this causes goroutine leak issue, and the resources (memory) associated with that cannot be released.

One information from pprof is shown below. In this case, performInstall successes to send resultMessage to channel, and handleContext is stuck there in reportToRun (where blocked by channel injection operation).

(pprof) top20                                                                                                              bytes
Showing nodes accounting for 63, 100% of 63 total                                                                          
Showing top 20 nodes out of 50                                                                                                
      flat  flat%   sum%        cum   cum%                                                                                   
........
         0     0%   100%          1  1.59%  helm.sh/helm/v3/pkg/action.(*Install).handleContext.func1                          
         0     0%   100%          1  1.59%  helm.sh/helm/v3/pkg/action.(*Install).reportToRun                                  

Could you help check this issue? Maybe one potential change is to declare this channel with length of two?
Let me know if any more information is needed.

@MaxCaresYww
Copy link
Author

It seems this PR will fix this: https://github.com/helm/helm/pull/10486/files

@zonggen zonggen added question/support bug Categorizes issue or PR as related to a bug. and removed question/support labels Dec 21, 2021
@zonggen
Copy link
Member

zonggen commented Dec 21, 2021

Reference for future readers, fix in the upgrade action was addressed in #10439.

@github-actions
Copy link

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Categorizes issue or PR as related to a bug. Stale
Projects
None yet
Development

No branches or pull requests

2 participants