Skip to content

ethmonitor: add poc for goroutine leak during shutdown #197

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.

Sign up for GitHub

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

Merged
xiam merged 5 commits into from
Jan 26, 2026
Merged

ethmonitor: add poc for goroutine leak during shutdown #197

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b8be9dc
ethmonitor: add poc for goroutine leak during shutdown
xiam Jan 21, 2026
8734b3b
ethmonitor: fixes after code review
xiam Jan 21, 2026
218cfec
ethmonitor: fix goroutine leak
xiam Jan 22, 2026
9f78a00
rename PoC test to TestMonitorShutdownNoGoroutineLeak
xiam Jan 22, 2026
2003fb6
ethmonitor: fix goroutine leak
xiam Jan 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 65 additions & 0 deletions ethmonitor/goroutine_leak_poc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
package ethmonitor_test

//go:generate mockgen -destination=internal/mocks/mock_provider.go -package=mocks github.com/0xsequence/ethkit/ethrpc RawInterface

import (
"context"
"fmt"
"math/big"
"runtime"
"testing"
"time"

"github.com/0xsequence/ethkit/ethmonitor"
"github.com/0xsequence/ethkit/ethmonitor/internal/mocks"
"go.uber.org/mock/gomock"
)

// TestMonitorShutdownGoroutineLeak demonstrates a goroutine leak on shutdown.
func TestMonitorShutdownGoroutineLeak(t *testing.T) {
Comment thread
xiam marked this conversation as resolved.
Outdated
if testing.Short() {
t.Skip("Skipping in short mode")
}

ctrl := gomock.NewController(t)
defer ctrl.Finish()

provider := mocks.NewMockRawInterface(ctrl)
provider.EXPECT().ChainID(gomock.Any()).Return(big.NewInt(1), nil).AnyTimes()
provider.EXPECT().IsStreamingEnabled().Return(false).AnyTimes()
provider.EXPECT().RawBlockByNumber(gomock.Any(), gomock.Any()).
Return(nil, fmt.Errorf("simulated network error")).AnyTimes()

initialGoroutines := runtime.NumGoroutine()

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The goroutine count baseline is captured before the test's own goroutine (lines 47-49) is started. This can lead to false positives in the leak detection. The baseline should be captured after starting the monitor goroutine, or the test should account for the test goroutine itself. Consider moving this line after line 49, or adding a small delay to ensure all test infrastructure goroutines are stable.

Copilot uses AI. Check for mistakes.

opts := ethmonitor.DefaultOptions
opts.PollingInterval = 10 * time.Millisecond
opts.Timeout = 50 * time.Millisecond

monitor, err := ethmonitor.NewMonitor(provider, opts)
if err != nil {
t.Fatalf("failed to create monitor: %v", err)
}

ctx, cancel := context.WithCancel(context.Background())

done := make(chan error, 1)
go func() {
done <- monitor.Run(ctx)
}()

time.Sleep(200 * time.Millisecond)

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The sleep duration of 200ms is arbitrary and may be insufficient on slower systems to reliably trigger the goroutine leak, or may be excessive on faster systems. Consider either increasing the duration to ensure the monitor enters its run loop, or using a more deterministic synchronization mechanism such as checking monitor.IsRunning() in a loop with a timeout.

Suggested change
time.Sleep(200 * time.Millisecond)
waitCtx, waitCancel := context.WithTimeout(context.Background(), time.Second)
defer waitCancel()
for {
if monitor.IsRunning() {
break
}
select {
case <-waitCtx.Done():
t.Fatalf("monitor did not start running within timeout: %v", waitCtx.Err())
case <-time.After(10 * time.Millisecond):
}
}

Copilot uses AI. Check for mistakes.

@xiam xiam Jan 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not worth it in this context.

cancel()

select {
case <-done:
case <-time.After(5 * time.Second):
t.Fatal("Monitor.Run() didn't return within timeout")
}

time.Sleep(500 * time.Millisecond)

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The test relies on hard-coded sleep durations which can cause flakiness in CI environments or under load. The 200ms sleep before cancellation (line 51) and the 500ms sleep after shutdown (line 60) may not be sufficient in all environments. Consider using synchronization primitives or runtime.GC() followed by a shorter sleep to ensure goroutines have exited, rather than relying solely on fixed time delays.

Suggested change
time.Sleep(500 * time.Millisecond)
// Wait for goroutines to exit, allowing for GC and load variability.
const (
maxWait = 5 * time.Second
pollDelay = 50 * time.Millisecond
)
deadline := time.Now().Add(maxWait)
for {
runtime.GC()
time.Sleep(pollDelay)
if runtime.NumGoroutine() <= initialGoroutines || time.Now().After(deadline) {
break
}
}

Copilot uses AI. Check for mistakes.

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The test doesn't explicitly call runtime.GC() and give goroutines time to be garbage collected before checking for leaks. The Go runtime may not immediately clean up exited goroutines. Consider adding runtime.GC() and a small sleep (e.g., 100ms) before the leak check to ensure more accurate results.

Suggested change
// Force garbage collection and give it a moment to complete before checking for leaks.
runtime.GC()
time.Sleep(100 * time.Millisecond)

Copilot uses AI. Check for mistakes.
Comment on lines +61 to +62

Copilot AI Jan 21, 2026

Copy link

Choose a reason for hiding this comment

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

The sleep duration of 100ms after GC may be insufficient for goroutines to fully clean up. Goroutine leak detection is inherently racy, and a more robust approach would be to poll runtime.NumGoroutine() in a loop with a reasonable timeout (e.g., 1 second) until the count returns to baseline or the timeout expires.

Suggested change
time.Sleep(100 * time.Millisecond)
// Poll for goroutine count to return to baseline with a reasonable timeout
deadline := time.Now().Add(1 * time.Second)
for time.Now().Before(deadline) {
if runtime.NumGoroutine() <= baseline {
break
}
time.Sleep(10 * time.Millisecond)
}

Copilot uses AI. Check for mistakes.
if leaked := runtime.NumGoroutine() - initialGoroutines; leaked > 0 {
t.Fatalf("%d goroutine(s) leaked", leaked)
}
}
Loading
Loading