fix(eth): reorder ethereum shutdown sequence#2277
fix(eth): reorder ethereum shutdown sequence#2277gzliudan wants to merge 1 commit intoXinFinOrg:dev-upgradefrom
Conversation
Close the ethereum shutdown channel before closing the chain database so bloom handler goroutines can exit first. Add stop-step logs around the shutdown path to pinpoint hangs during node exit.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Reorders parts of the Ethereum service shutdown sequence to signal bloom-bits handler goroutines earlier (before closing the chain DB) and adds additional stop-step logging to help diagnose node exit hangs.
Changes:
- Move
close(e.shutdownChan)to occur beforee.chainDb.Close(). - Add start/stop log lines around each shutdown step (bloom indexer, blockchain, protocol manager, txpool, miner, event mux, DB).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| log.Info("Stopping Ethereum bloomIndexer start") | ||
| e.bloomIndexer.Close() | ||
| log.Info("Ethereum bloomIndexer stopped") |
There was a problem hiding this comment.
e.bloomIndexer.Close() returns an error, but it’s currently ignored while logs unconditionally report it as “stopped”. Since Ethereum.Stop() has an error return, consider capturing/propagating the close error (or at least logging it) so shutdown failures aren’t silently missed.
| log.Info("Stopping Ethereum shutdownChan start") | ||
| close(e.shutdownChan) | ||
| log.Info("Ethereum shutdownChan stopped") | ||
|
|
||
| log.Info("Stopping Ethereum miner start") | ||
| e.miner.Stop() | ||
| log.Info("Ethereum miner stopped") | ||
|
|
||
| log.Info("Stopping Ethereum eventMux start") | ||
| e.eventMux.Stop() | ||
| log.Info("Ethereum eventMux stopped") | ||
|
|
||
| log.Info("Stopping Ethereum chainDb start") | ||
| e.chainDb.Close() |
There was a problem hiding this comment.
Closing shutdownChan only signals the bloom service goroutines to exit; it doesn’t ensure they’ve actually stopped before chainDb.Close() runs. In startBloomHandlers the goroutine may still be mid-task (inner for over sections) and continue reading from eth.chainDb. If the intent is to let bloom handlers exit before closing the DB (and avoid shutdown hangs), consider tracking these goroutines with a sync.WaitGroup and waiting after close(e.shutdownChan) (and/or checking shutdownChan inside the per-section loop) prior to closing chainDb.
| log.Info("Ethereum eventMux stopped") | ||
|
|
||
| log.Info("Stopping Ethereum chainDb start") | ||
| e.chainDb.Close() |
There was a problem hiding this comment.
e.chainDb.Close() (io.Closer) can return an error, but it’s ignored and the subsequent log line reports the DB as “stopped” regardless. Consider handling/logging the returned error and (optionally) returning it from Ethereum.Stop() so callers can detect shutdown failures.
| e.chainDb.Close() | |
| if err := e.chainDb.Close(); err != nil { | |
| log.Error("Failed to stop Ethereum chainDb", "err", err) | |
| return err | |
| } |
Proposed changes
Close the ethereum shutdown channel before closing the chain database so bloom handler goroutines can exit first. Add stop-step logs around the shutdown path to pinpoint hangs during node exit.
some log messages about abnormal exit:
Types of changes
What types of changes does your code introduce to XDC network?
Put an
✅in the boxes that applyImpacted Components
Which parts of the codebase does this PR touch?
Put an
✅in the boxes that applyChecklist
Put an
✅in the boxes once you have confirmed below actions (or provide reasons on not doing so) that