Skip to content

ethstats: avoid creating subscriptions on background goroutine#22587

Merged
fjl merged 4 commits into
ethereum:masterfrom
MariusVanDerWijden:ethstats-nil-subscription
Mar 30, 2021
Merged

ethstats: avoid creating subscriptions on background goroutine#22587
fjl merged 4 commits into
ethereum:masterfrom
MariusVanDerWijden:ethstats-nil-subscription

Conversation

@MariusVanDerWijden

Copy link
Copy Markdown
Member

No description provided.

Comment thread ethstats/ethstats.go Outdated
Comment thread ethstats/ethstats.go Outdated
if headSub == nil {
log.Info("Stats daemon stopped due to nil head subscription")
return
}

@fjl fjl Mar 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's easy to avoid this error by subscribing earlier. If you create the subscriptions in New (or Start) they can never be nil.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If they're created in Start, then the deferred Unsubscribe will become "decoupled" from the Subscribe, which is not great either...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The Unsubscribe would need to move into Service.Stop then.

@holiman holiman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

Comment thread ethstats/ethstats.go Outdated
Comment thread ethstats/ethstats.go Outdated
Co-authored-by: Martin Holst Swende <martin@swende.se>

@holiman holiman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@fjl fjl merged commit 3faae5d into ethereum:master Mar 30, 2021
@fjl fjl changed the title ethstats: stop loop if subscription fail ethstats: avoid creating subscriptions on background goroutine Mar 30, 2021
atif-konasl pushed a commit to frozeman/pandora-execution-engine that referenced this pull request Oct 15, 2021
…eum#22587)

This fixes an issue where the ethstats service could crash if geth was
started and then immediately stopped due to an internal error. The
cause of the crash was a nil subscription being returned by the backend,
because the background goroutine creating them was scheduled after
the backend had already shut down.

Moving the creation of subscriptions into the Start method, which runs
synchronously during startup of the node, means the returned subscriptions
can never be 'nil'.

Co-authored-by: Felix Lange <fjl@twurst.com>
Co-authored-by: Martin Holst Swende <martin@swende.se>
@MariusVanDerWijden MariusVanDerWijden deleted the ethstats-nil-subscription branch November 30, 2021 15:25
gzliudan added a commit to gzliudan/XDPoSChain that referenced this pull request Sep 8, 2025
gzliudan added a commit to XinFinOrg/XDPoSChain that referenced this pull request Sep 8, 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