runtime: unreachable stopped time.AfterFunc Timers retain their arg payload

[bradfitz]

[Go 1.25.3, macOS+Linux]

Found while debugging a production memory ~leak....

https://go.dev/play/p/dQaXA0miYoX

If you make a time.AfterFunc and Stop it, it doesn't zero out its runtime.timer.arg any interface value because you might Reset it later. But if it's Stopped and the P-heaped timer entry is zombied and the *time.Timer is no longer GC-reachable, that zombied heap entry still has that non-zero arg interface until the timer fires. (or until it becomes the min/max in the heap and cleaned)

In the repro above, it puts it in the middle of the heap, so it's not cleaned.

Can we make runtime.newTimer use AddCleanup or something (on the timeTimer wrapper around the runtime.timer) to zero out the arg field for stopped zombie timers?

[neild]

I don't think we can put an AddCleanup on the timer, because the heap contains a reference to the timer and will keep it alive.

Maybe the GC can clean the timer heaps at some point?

[bradfitz]

Does timeTimer need to embed timer?

[neild]

So long as we're okay with an extra allocation per Timer, I don't think timeTimer needs to embed timer.

[gabyhelp]

Related Issues

• time: stop requiring Timer/Ticker.Stop for prompt GC #61542 (closed)

Related Code Changes

• runtime, time: arrange to stop tickers when they are garbage collected

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[bradfitz]

My naive attempt fan afoul of runtime rules.... 😅

go/src/runtime/mcleanup.go:74:35: cleanup escapes to heap, not allowed in runtime
go/src/runtime/mcleanup.go:74:52: arg escapes to heap, not allowed in runtime
go/src/runtime/mcleanup.go:101:8: heap-allocated closure AddCleanup[go.shape.struct { runtime.c unsafe.Pointer; runtime.init bool; runtime.timer },go.shape.*uint8].func1, not allowed in runtime

diff --git a/src/runtime/time.go b/src/runtime/time.go
index e9d1f0b6c9..09aa799e45 100644
--- a/src/runtime/time.go
+++ b/src/runtime/time.go
@@ -406,6 +406,8 @@ func newTimer(when, period int64, f func(arg any, seq uintptr, delay int64), arg
        }
        t.modify(when, period, f, arg, 0)
        t.init = true
+
+       AddCleanup(t, (*timer).zeroIfStopped, &t.timer)
        return t
 }
 
@@ -490,6 +492,14 @@ func (t *timer) maybeRunAsync() {
        }
 }
 
+func (t *timer) zeroIfStopped() {
+       t.lock()
+       if t.state&timerZombie != 0 {
+               t.arg = nil
+       }
+       t.unlock()
+}
+
 // stop stops the timer t. It may be on some other P, so we can't
 // actually remove it from the timers heap. We can only mark it as stopped.
 // It will be removed in due course by the P whose heap it is on.

[ianlancetaylor]

Currently the goroutine scheduler will call pp.timers.check each time it looks for a new goroutine to run. With the current implementation, if there are any timers to run, and if more than 1/4 of the timers are zombies, then it will remove them from the timer heap by calling (*timers).adjust with force set to true.

Your test case adds a bunch of timers that are not zombies, so it's not the case that 1/4 of the timers are zombies, so (*timers).adjust is not run.

So in general we can fix this problem if we call (*timers).adjust (or a simpler version of that method) for each timer heap, with force set to true. As @neild suggests, perhaps we can do that during each GC cycle. In particular, we only need to do it when there are zombies, which is tracked in timers.zombies.

[bradfitz]

(Related issue coming out of all this, to add metrics: #75956)