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: don't wait for snap registration if we're not running snap #22272

Merged
merged 3 commits into from
Feb 5, 2021

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Feb 5, 2021

This PR fixes a hang-on-shutdown, due to the new logic where we wait for a successful snap registration. The problem was that if we don't ourself run snap, then we'll just wait forever.

The somewhat naive fix here just adds a flag to the eth handler, to make it aware of whether the snap protocol is even enabled.

More context / logs: https://gist.github.com/holiman/22d42503cd6620fdee0baf00d6a857f3

eth/handler.go Outdated
peer.Log().Error("Snapshot extension barrier failed", "err", err)
return err
var snapPeer *snap.Peer
if peer.RunningCap(snap.ProtocolName, snap.ProtocolVersions) {
Copy link
Member

Choose a reason for hiding this comment

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

This check is not necessary here. The original code is fine, will return nil if snap is not enabled. No need to check it here too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right -- I added this first, and later changed the inner one. and didn't consider that the inner change made this moot

Copy link
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

SGTM

@karalabe karalabe added this to the 1.10.0 milestone Feb 5, 2021
@holiman
Copy link
Contributor Author

holiman commented Feb 5, 2021

Build failed twice in a row on

--- FAIL: TestClientCancelWebsocket (0.81s)
panic: read tcp 127.0.0.1:40502->127.0.0.1:44959: read: connection reset by peer [recovered]
	panic: read tcp 127.0.0.1:40502->127.0.0.1:44959: read: connection reset by peer

Screw that, merge incoming

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.

2 participants