warmuper.WaitAndClose: cancel work before wait#20332
Conversation
before: - seems we never close Warmuper - because WaitAndClose switching atomic. - seems we wait-first and then cancel/close work channel - it's wrong because `WaitAndClose()` called when warmup not needed anymore (commitment calc finished): ``` warmuper.Start() defer warmuper.WaitAndClose() ``` PR: canceling first and then wait gorouines to finish
There was a problem hiding this comment.
Pull request overview
This PR adjusts warmup worker shutdown semantics so warmup work is canceled before waiting for goroutines to exit, addressing cases where warmup is no longer needed (e.g., deferred cleanup during commitment processing).
Changes:
- Replace
Warmuper.WaitAndClose()withWarmuper.CloseAndWait()that callsClose()first, then waits for worker goroutines. - Update
HexPatriciaHashed.Processto deferCloseAndWait()instead ofWaitAndClose().
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
execution/commitment/warmuper.go |
Renames/reworks shutdown helper to cancel work before waiting on workers. |
execution/commitment/hex_patricia_hashed.go |
Switches deferred warmuper cleanup to the new shutdown helper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // CloseAndWait cancel and waits for all warmup work | ||
| func (w *Warmuper) CloseAndWait() { | ||
| w.Close() | ||
| if w.g != nil { | ||
| _ = w.g.Wait() | ||
| } |
There was a problem hiding this comment.
CloseAndWait calls w.Close() unconditionally. If Start() returned early (e.g., cfg.NumWorkers <= 0), w.work is never initialized and Close() will panic when it tries to close a nil channel. Consider making Close()/CloseAndWait() safe when not started (e.g., guard on w.work != nil / w.g != nil, or restructure Start() so work is always initialized before Close can run).
| return // Already closed | ||
| } | ||
| w.Wait() | ||
| // CloseAndWait cancel and waits for all warmup work |
There was a problem hiding this comment.
The doc comment has grammatical issues: "CloseAndWait cancel and waits for all warmup work" should be something like "CloseAndWait cancels and waits for all warmup work" (and ideally mention that it cancels in-flight warmup rather than draining it).
| // CloseAndWait cancel and waits for all warmup work | |
| // CloseAndWait cancels warmup work and waits for in-flight workers to exit. |
| // CloseAndWait cancel and waits for all warmup work | ||
| func (w *Warmuper) CloseAndWait() { | ||
| w.Close() | ||
| if w.g != nil { | ||
| _ = w.g.Wait() | ||
| } |
There was a problem hiding this comment.
This PR changes warmuper shutdown semantics (cancel-before-wait) and introduces a new public method CloseAndWait, but there doesn't appear to be test coverage exercising Process() with warmup.Enabled=true (existing hex_patricia_hashed tests pass WarmupConfig{}), nor a unit test that CloseAndWait is safe/idempotent. Adding a test that enables warmup and asserts CloseAndWait doesn't leak goroutines / doesn't panic (including NumWorkers=0 edge case) would help prevent regressions.
…x, xsync fix, warmuper fix, logs debug, unused code (#20337) Cherry-picks from release/3.4 to main: - integrity: blk-range chk to pre-build "changed keys" index before calc state root (#20302) - up gql and grpc (#20304) - stepSize: all tooling to use `tx.Debug().StepSize()` instead of `DefaultStepSize` constant (#20280) - db/state: fix CursorHeap tie-break to prefer RAM over DB over FILE (#20318) - xsync deprecated use fix (#20331) - warmuper.WaitAndClose: cancel work before wait (#20332) - logs: move some Info logs to Debug level (to simplify logs for Users) (#20329) - db: unused code remove (#20334) --------- Co-authored-by: moskud <sudeepdino008@gmail.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
before:
WaitAndClose()called when warmup not needed anymore (commitment calc finished):PR: canceling first and then wait gorouines to finish