Skip to content

refactor: use WaitGroup.Go to simplify code#2828

Open
purelualight wants to merge 1 commit into
ssvlabs:stagefrom
purelualight:stage
Open

refactor: use WaitGroup.Go to simplify code#2828
purelualight wants to merge 1 commit into
ssvlabs:stagefrom
purelualight:stage

Conversation

@purelualight
Copy link
Copy Markdown

Adopt sync.WaitGroup.Go (Go 1.25) to simplify tracked goroutine spawning.

This replaces the error-prone trio of wg.Add(1), go func(), and defer wg.Done() with a single, self-contained call.

More info: golang/go#63796

@purelualight purelualight requested review from a team as code owners April 23, 2026 13:25
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 23, 2026

Greptile Summary

This PR adopts sync.WaitGroup.Go (introduced in Go 1.25) across 18 files to replace the boilerplate wg.Add(1) / go func() { defer wg.Done() ... }() pattern. The project's go.mod already declares go 1.26, so the new API is available. All 30+ replacements are semantically equivalent — wg.Go(f) is defined as wg.Add(1); go func() { defer wg.Done(); f() }() — making this a safe, mechanical refactor with no behaviour changes.

Confidence Score: 5/5

Safe to merge — purely mechanical refactor with no logic changes and confirmed API availability in Go 1.25+.

All replacements are semantically identical to the original code; the Go version constraint (1.26) guarantees WaitGroup.Go availability; no new logic is introduced.

No files require special attention.

Important Files Changed

Filename Overview
cli/operator/node.go Replaces Add(1)/go func()/defer Done() trio with wg.Go() in slot pruning loop — semantically equivalent.
operator/duties/scheduler.go Five goroutine launches refactored to wg.Go() — duty execution and background tasks all equivalent.
network/topics/controller_test.go Outer goroutine uses wg.Go(); body still calls wg.Add(1) for inner goroutines — pre-existing pattern, semantically equivalent, correct because Add() is called before the outer wg.Done().
protocol/v2/ssv/runner/committee.go Worker goroutine loop refactored to wg.Go() — no semantic change.
operator/duties/attester.go Background subscription goroutine converted to wg.Go() — equivalent.
operator/duties/sync_committee.go Background subscription goroutine converted to wg.Go() — equivalent.
hprobe/prober.go Concurrent probe launches converted to wg.Go() — equivalent.
utils/hashmap/hashmap_test.go Nested wg.Go() usage (outer wwg.Go wraps inner wg.Go loop) — pre-existing pattern, correctly equivalent.
utils/ttl/map_test.go Mirrors hashmap_test.go changes with nested wg.Go() — equivalent.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant WaitGroup
    participant Goroutine

    Note over Caller,Goroutine: Old pattern
    Caller->>WaitGroup: Add(1)
    Caller->>Goroutine: launch goroutine with defer Done
    Goroutine-->>WaitGroup: Done deferred on return
    Caller->>WaitGroup: Wait

    Note over Caller,Goroutine: New pattern via WaitGroup.Go
    Caller->>WaitGroup: Go(f)
    WaitGroup->>WaitGroup: Add(1) internally
    WaitGroup->>Goroutine: launch goroutine
    Goroutine-->>WaitGroup: Done internally on return
    Caller->>WaitGroup: Wait
Loading

Reviews (1): Last reviewed commit: "refactor: use WaitGroup.Go to simplify c..." | Re-trigger Greptile

nkryuchkov
nkryuchkov previously approved these changes Apr 24, 2026
Copy link
Copy Markdown
Contributor

@nkryuchkov nkryuchkov left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution

ljuba-ssv
ljuba-ssv previously approved these changes Apr 27, 2026
Comment thread operator/duties/scheduler.go
iurii-ssv
iurii-ssv previously approved these changes Apr 27, 2026
Copy link
Copy Markdown
Contributor

@iurii-ssv iurii-ssv left a comment

Choose a reason for hiding this comment

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

Good stuff, @purelualight can you also fix the failed linter pipeline ?

@momosh-ssv momosh-ssv self-requested a review April 30, 2026 15:49
wg.Go(func() {
// check number of peers and messages
for i := 0; i < nValidators; i++ {
wg.Add(1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What do you think about using a separate innerWg for these goroutines? Seems that calling wg.Add(1) from inside a closure that's itself counted by the same wg is fragile — it works today because the for-loop always runs to completion before the outer goroutine returns, but a future early-return (e.g. on ctx cancel) could let the trailing wg.Wait() unblock before all inner Add calls land, racing any subsequent reuse of the same wg.

Comment thread hprobe/prober.go Outdated
@purelualight
Copy link
Copy Markdown
Author

Good stuff, @purelualight can you also fix the failed linter pipeline ?

@iurii-ssv @nkryuchkov Thank you for your review. I’ve resolved the conflicts and fixed the lint issues (I removed the extra blank lines). Please review it again.

Signed-off-by: purelualight <purelualight@outlook.com>
@iurii-ssv
Copy link
Copy Markdown
Contributor

@purelualight the CI linter seems to have failed again, can you run make lint locally to figure out why and fix it ?

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.

5 participants