Skip to content

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

Merged
unclezoro merged 1 commit intobnb-chain:developfrom
j75689:fix/protocol_issue
Jul 26, 2022
Merged

[R4R] fix: memory leak issue with diff protocol#1019
unclezoro merged 1 commit intobnb-chain:developfrom
j75689:fix/protocol_issue

Conversation

@j75689
Copy link
Copy Markdown
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.

brilliant-lx
brilliant-lx approved these changes Jul 26, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

forcodedancing commented Jul 26, 2022

LGTM

This was referenced Jul 28, 2022
Merged
Merged
@0xe3b0c4 0xe3b0c4 mentioned this pull request Aug 27, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@unclezoro unclezoro unclezoro approved these changes

@forcodedancing forcodedancing forcodedancing approved these changes

@brilliant-lx brilliant-lx brilliant-lx approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@j75689 @forcodedancing @unclezoro @brilliant-lx