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

[R4R] fix: memory leak issue with diff protocol #1019

Merged
merged 1 commit into from
Jul 26, 2022

Conversation

j75689
Copy link
Contributor

@j75689 j75689 commented Jul 26, 2022

Description

broadcastDiffLayers is not properly closed, the memory accumulates too many resources and causes OOM.

goroutine 8845520 [select, 3426 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/diff.(*Peer).broadcastDiffLayers(0xc081628d20)
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:47 +0xab
created by github.com/ethereum/go-ethereum/eth/protocols/diff.NewPeer
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:41 +0x2f0

goroutine 7119763 [select, 3531 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/diff.(*Peer).broadcastDiffLayers(0xc10bcd4e40)
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:47 +0xab
created by github.com/ethereum/go-ethereum/eth/protocols/diff.NewPeer
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:41 +0x2f0

goroutine 11443031 [select, 3272 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/diff.(*Peer).broadcastDiffLayers(0xc0f9bb3260)
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:47 +0xab
created by github.com/ethereum/go-ethereum/eth/protocols/diff.NewPeer
	github.com/ethereum/go-ethereum/eth/protocols/diff/peer.go:41 +0x2f0

Rationale

broadcastDiffLayers will be started in NewPeer; need to call peer.Close when RunPeer fails. otherwise, this goroutine will not be closed.

Example

panic.log

Changes

Notable changes:

  • close the peer on failure in RunPeer function.

Copy link
Collaborator

@brilliant-lx brilliant-lx left a comment

Choose a reason for hiding this comment

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

LGTM

@unclezoro unclezoro changed the title [R4R] memory leak issue with diff protocol [R4R] fix: memory leak issue with diff protocol Jul 26, 2022
@unclezoro unclezoro merged commit dd3b3a6 into bnb-chain:develop Jul 26, 2022
@forcodedancing
Copy link
Contributor

LGTM

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.

4 participants