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

fix logic issue: handlers.removePeer() is called twice. #856

Merged
merged 4 commits into from
Apr 24, 2022
Merged

fix logic issue: handlers.removePeer() is called twice. #856

merged 4 commits into from
Apr 24, 2022

Conversation

Jolly23
Copy link
Contributor

@Jolly23 Jolly23 commented Apr 12, 2022

Description

There is a logic issue which causes "Ethereum peer removal failed, err=peer not registered" occur quite often.
Related issues #220 #670 #821

Rationale

handler.runEthPeer() set up a defer removePeer(). This is always called after a peer is disconnected.
However removePeer is also called by multiple functions like downloader/fetcher. After those kind of functions call removePeer(), peer handler executes defer removePeer(). This makes removePeer() happened twice, and this is the reason we often see "Ethereum peer removal failed, err=peer not registered".

Just took a look, go-ethereum code here is exactly using this new logic

Changes

Notable changes:

  • removePeer() hard Disconnect peer from networking layer.
  • defer unregisterPeer() will called by handler.runEthPeer(), do the cleanup task after then.
  • add NewPeerPipe() under p2p.Peer, for RW close testing. The code is referenced from go-ethereum-p2p-peer

unclezoro and others added 4 commits January 28, 2022 11:44
There is a logic issue which cause "Ethereum peer removal failed, err=peer not registered" occur quite often.

handler.runEthPeer set up a defer removePeer(). This is always called after a peer is disconnected.
However removePeer is also called by mulitple functions like downloader/fetcher.  After those kind of functions removePeer(), peer handler executes defer removePeer(). This makes removePeer() happened twice, and this is the reason we often see "Ethereum peer removal failed, err=peer not registered".

To solve this, removePeer only needs to hard Disconnect peer from networking layer. Then defer unregisterPeer() will do the cleanup task after then.
@unclezoro
Copy link
Collaborator

Thanks for the contribution. We will start review ASAP.

@unclezoro unclezoro changed the base branch from master to develop April 20, 2022 04:09
@unclezoro
Copy link
Collaborator

LGTM

@Jolly23
Copy link
Contributor Author

Jolly23 commented Apr 20, 2022

Related issues #220 #670 #821

Copy link
Contributor

@keefel keefel 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 merged commit 15bc254 into bnb-chain:develop Apr 24, 2022
baptiste-b-pegasys added a commit to baptiste-b-pegasys/quorum that referenced this pull request May 27, 2022
achraf17 pushed a commit to Consensys/quorum that referenced this pull request May 30, 2022
j75689 pushed a commit to j75689/bsc that referenced this pull request Jun 15, 2022
* fix logic issue: handlers.removePeer() is called twice.

There is a logic issue which cause "Ethereum peer removal failed, err=peer not registered" occur quite often.

handler.runEthPeer set up a defer removePeer(). This is always called after a peer is disconnected.
However removePeer is also called by mulitple functions like downloader/fetcher.  After those kind of functions removePeer(), peer handler executes defer removePeer(). This makes removePeer() happened twice, and this is the reason we often see "Ethereum peer removal failed, err=peer not registered".

To solve this, removePeer only needs to hard Disconnect peer from networking layer. Then defer unregisterPeer() will do the cleanup task after then.

* fix: modify test function for close testing.

reference from go-thereum.

Co-authored-by: zjubfd <[email protected]>
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.

5 participants