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

eth/protocols/eth: prevent hanging dispatch #30918

Merged
merged 2 commits into from
Jan 6, 2025
Merged

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Dec 16, 2024

This PR attempts to fix a strange test-failure (timeout) observed on a windows-32 platform.
https://ci.appveyor.com/project/ethereum/go-ethereum/builds/51174391/job/d8ascanwwltrlqd5

A goroutine is stuck trying to deliver a response:

goroutine 9632 [select, 29 minutes]:
github.com/ethereum/go-ethereum/eth/protocols/eth.(*Peer).dispatchResponse(0x314f100, 0x3e5f6d0, 0x3acbb84)
	C:/projects/go-ethereum/eth/protocols/eth/dispatcher.go:172 +0x2a5
github.com/ethereum/go-ethereum/eth/protocols/eth.handleBlockHeaders({0x12abe68, 0x30021b8}, {0x12a815c, 0x40b41c0}, 0x314f100)
	C:/projects/go-ethereum/eth/protocols/eth/handlers.go:301 +0x173
github.com/ethereum/go-ethereum/eth/protocols/eth.handleMessage({0x12abe68, 0x30021b8}, 0x314f100)
	C:/projects/go-ethereum/eth/protocols/eth/handler.go:205 +0x4f6
github.com/ethereum/go-ethereum/eth/protocols/eth.Handle({0x12abe68, 0x30021b8}, 0x314f100)
	C:/projects/go-ethereum/eth/protocols/eth/handler.go:149 +0x33
github.com/ethereum/go-ethereum/eth.testSnapSyncDisabling.func1(0x314f100)
	C:/projects/go-ethereum/eth/sync_test.go:65 +0x33
github.com/ethereum/go-ethereum/eth.(*handler).runEthPeer(0x30021b8, 0x314f100, 0x427f648)
	C:/projects/go-ethereum/eth/handler.go:355 +0xe65
created by github.com/ethereum/go-ethereum/eth.testSnapSyncDisabling in goroutine 11
	C:/projects/go-ethereum/eth/sync_test.go:64 +0x54f
FAIL	github.com/ethereum/go-ethereum/eth	1800.138s

I do not know if this is the proper fix, but it seems like a reasonable one to me. Ptal @fjl

@fjl fjl self-assigned this Dec 16, 2024
rjl493456442
rjl493456442 previously approved these changes Dec 17, 2024
Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

SGTM

@rjl493456442
Copy link
Member

Btw, the TestSnapSyncDisabling68 is super weird, I will push something to improve the test in your PR.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

Let's merge it once the CI is green

@fjl
Copy link
Contributor

fjl commented Dec 18, 2024

I kind of want to check it before merging, just didn't have time yet.

@fjl fjl merged commit 9298074 into ethereum:master Jan 6, 2025
3 checks passed
@fjl fjl added this to the 1.14.13 milestone Jan 6, 2025
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.

3 participants