mapper: correct the send timer rationale

time.After has been GC-recoverable since Go 1.23, so the leak the comment
described cannot happen and the test asserting it proved nothing.
This commit is contained in:
Kristoffer Dalby
2026-08-25 09:44:03 +00:00
committed by Kristoffer Dalby
parent 635bda9139
commit d7c1ecce92
2 changed files with 3 additions and 48 deletions
+1 -44
View File
@@ -950,52 +950,9 @@ func TestMultiChannelSend_ConcurrentRemoveAndSend(t *testing.T) {
}
// ============================================================================
// Regression tests for H1 (timer leak) and H3 (lifecycle)
// Regression test for H3 (lifecycle)
// ============================================================================
// TestConnectionEntry_SendFastPath_TimerStopped is a regression guard for H1.
// Before the fix, connectionEntry.send used time.After(50ms) which leaked a
// timer into the runtime heap on every call even when the channel send
// succeeded immediately. The fix switched to time.NewTimer + defer Stop().
//
// This test sends many messages on a buffered (non-blocking) channel and
// checks that the number of live goroutines stays bounded, which would
// grow without bound under the old time.After approach at high call rates.
func TestConnectionEntry_SendFastPath_TimerStopped(t *testing.T) {
const sends = 5000
ch := make(chan *tailcfg.MapResponse, sends)
entry := &connectionEntry{
id: "timer-leak-test",
c: ch,
version: 100,
created: time.Now(),
}
resp := testMapResponse()
for range sends {
err := entry.send(resp)
require.NoError(t, err)
}
// Drain the channel so we aren't holding references.
for range sends {
<-ch
}
// Force a GC + timer cleanup pass.
runtime.GC()
// If timers were leaking we'd see a goroutine count much higher
// than baseline. With 5000 leaked timers the count would be
// noticeably elevated. We just check it's reasonable.
numGR := runtime.NumGoroutine()
assert.Less(t, numGR, 200,
"goroutine count after %d fast-path sends should be bounded; got %d (possible timer leak)", sends, numGR)
}
// TestBatcher_CloseWaitsForWorkers is a regression guard for H3.
// Before the fix, Close() would tear down node connections while workers
// were potentially still running, risking sends on closed channels.
+2 -4
View File
@@ -403,10 +403,8 @@ func (entry *connectionEntry) send(data *tailcfg.MapResponse) error {
// This is critical for detecting Docker containers that are forcefully terminated
// but still have channels that appear open.
//
// We use time.NewTimer + Stop instead of time.After to avoid leaking timers.
// time.After creates a timer that lives in the runtime's timer heap until it fires,
// even when the send succeeds immediately. On the hot path (1000+ nodes per tick),
// this leaks thousands of timers per second.
// Use a timer rather than time.After so the timeout is explicitly released
// on the fast path; both are GC-recoverable since Go 1.23.
timer := time.NewTimer(50 * time.Millisecond) //nolint:mnd
defer timer.Stop()