Skip to content

[R4R] add timeout for stopping p2p server#643

Merged
yutianwu merged 4 commits intodevelopfrom
fix_p2p_stop
Dec 13, 2021
Merged

[R4R] add timeout for stopping p2p server#643
yutianwu merged 4 commits intodevelopfrom
fix_p2p_stop

Conversation

@yutianwu
Copy link
Copy Markdown
Contributor

@yutianwu yutianwu commented Dec 8, 2021 •
edited by unclezoro
Loading

Description

For now, when accepting p2p peers in p2p server, we will run runEthPeer and RunPeer in diffHandler and snapHandler concurrently, so sometimes runEthPeer will hang there for waiting for the signal that it missed before it runninng.

This will cause we can not stop the bsc node gracefully, so we add a timeout for stopping p2p server and a timeout for handling p2p peers.

Rationale

When we can not stop the bsc node gracefully, we will see this in the profile:

goroutine 78355508 [chan receive, 100 minutes]:
github.com/ethereum/go-ethereum/eth.(*peerSet).waitDiffExtension(0xc000eb69b0, 0xc06931f5e0)
	github.com/ethereum/go-ethereum/eth/peerset.go:206 +0x17e
github.com/ethereum/go-ethereum/eth.(*handler).runEthPeer(0xc0031fe000, 0xc06931f5e0, 0xc193063f68)
	github.com/ethereum/go-ethereum/eth/handler.go:261 +0x19c
github.com/ethereum/go-ethereum/eth.(*ethHandler).RunPeer(0x43, 0xc2c490a600, 0x18c6a78)
	github.com/ethereum/go-ethereum/eth/handler_eth.go:46 +0x19
github.com/ethereum/go-ethereum/eth/protocols/eth.MakeProtocols.func1(0x47bcd9, {0x18c6a78, 0xc23a41eb40})
	github.com/ethereum/go-ethereum/eth/protocols/eth/handler.go:117 +0x122
github.com/ethereum/go-ethereum/p2p.(*Peer).startProtocols.func1()
	github.com/ethereum/go-ethereum/p2p/peer.go:396 +0x8c
created by github.com/ethereum/go-ethereum/p2p.(*Peer).startProtocols
	github.com/ethereum/go-ethereum/p2p/peer.go:394 +0xb3

Changes

Notable changes:

  • add a timeout for handling p2p peers
  • add a timeout for stop p2p server

j75689
j75689 reviewed Dec 8, 2021
View reviewed changes
j75689
j75689 reviewed Dec 8, 2021
View reviewed changes
@yutianwu yutianwu force-pushed the fix_p2p_stop branch 2 times, most recently from 09962cc to 75130cd Compare December 8, 2021 08:51
unclezoro
unclezoro reviewed Dec 8, 2021
View reviewed changes
@yutianwu yutianwu changed the title add timeout for stopping p2p server [R4R] add timeout for stopping p2p server Dec 9, 2021
@keefel
Copy link
Copy Markdown
Contributor

keefel commented Dec 9, 2021

LGTM

@keefel keefel self-requested a review December 13, 2021 03:14
@kyrie-yl kyrie-yl requested review from keefel and kyrie-yl and removed request for keefel December 13, 2021 03:14
@yutianwu yutianwu merged commit 7c1c8e2 into develop Dec 13, 2021
unclezoro pushed a commit that referenced this pull request Dec 20, 2021
@yutianwu @unclezoro
[R4R] add timeout for stopping p2p server (#643)
9e25fca 
* add timeout for stopping p2p server

* extend extension wait time

* add unit tests

* fix lint issue
unclezoro pushed a commit that referenced this pull request Dec 20, 2021
@yutianwu @unclezoro
[R4R] add timeout for stopping p2p server (#643)
151ee96 
* add timeout for stopping p2p server

* extend extension wait time

* add unit tests

* fix lint issue
keefel pushed a commit to keefel/bsc that referenced this pull request Dec 20, 2021
@yutianwu @keefel
[R4R] add timeout for stopping p2p server (bnb-chain#643)
91350fe 
* add timeout for stopping p2p server

* extend extension wait time

* add unit tests

* fix lint issue
This was referenced Jan 26, 2022
Merged
Merged
@unclezoro unclezoro deleted the fix_p2p_stop branch May 10, 2022 12:30
keefel pushed a commit to keefel/bsc that referenced this pull request Jun 6, 2022
@yutianwu @j75689
[R4R] add timeout for stopping p2p server (bnb-chain#643)
33a4607 
* add timeout for stopping p2p server

* extend extension wait time

* add unit tests

* fix lint issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kyrie-yl kyrie-yl Awaiting requested review from kyrie-yl

4 more reviewers

@unclezoro unclezoro unclezoro approved these changes

@j75689 j75689 j75689 approved these changes

@forcodedancing forcodedancing forcodedancing approved these changes

@keefel keefel keefel 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.

5 participants

@yutianwu @keefel @unclezoro @j75689 @forcodedancing