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

解决goroutine泄漏问题 #4638

Merged
merged 3 commits into from
Jan 16, 2025
Merged

Conversation

Alpha2J
Copy link
Contributor

@Alpha2J Alpha2J commented Jan 15, 2025

WHY

由于closeCh没有被初始化,在Close()函数中执行close(pxy.closeCh)的时候,会panic:"panic error: close of nil channel";导致无法退出Run()函数中创建的goroutine,造成goroutine泄漏,同时引发内存泄漏;在我的案例中,client用户单实例大概4000,运行两天,阻塞在case <-pxy.closeCh中的goroutine达到三万多;

@fatedier fatedier requested a review from Copilot January 15, 2025 09:40

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@fatedier
Copy link
Owner

func (pxy *XTCPProxy) Close() {
	pxy.BaseProxy.Close()
	pxy.rc.NatHoleController.CloseClient(pxy.GetName())
	_ = errors.PanicToError(func() {
		close(pxy.closeCh)
	})
}

Could you also update this to use sync.Once to ensure it only closes once? This way, we can avoid using PanicToError.

@Alpha2J
Copy link
Contributor Author

Alpha2J commented Jan 15, 2025

Could you also update this to use sync.Once to ensure it only closes once? This way, we can avoid using PanicToError.

Sure, I've updated the code to use sync.Once to ensure that close(pxy.closeCh) is only called once. However, I'm still not quite sure why close(pxy.closeCh)needs to be wrapped with PanicToError. Could you please clarify the reasoning behind that? Is it due to some historical reasons or edge cases that need to be handled?

@fatedier
Copy link
Owner

fatedier commented Jan 15, 2025

This way, we can avoid using PanicToError.

Just remove it.

@fatedier fatedier requested a review from Copilot January 16, 2025 02:49

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.

@fatedier fatedier merged commit 450b839 into fatedier:dev Jan 16, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants