add a linter for leaking Tickers – Ticker.Stop is missing

[srosenberg]

Background

The official API documentation [1] for time.NewTicker states,

Stop the ticker to release associated resources.

The included example illustrates correct usage, namely, defer,

ticker := time.NewTicker(time.Second)
defer ticker.Stop()

It may seem that GC would come to the rescue in cases where Stop is missing. After all, forgetting to Close the file descriptor from os.OpenFile doesn't result in a permanent resource leak. This is because runtime.SetFinalizer will eventually invoke Close on GCed file descriptors. Unfortunately, this doesn't hold for Ticker (or Timer) [2]. The runtime stores (active) timers in heap that's attached to a specific P, in the timers array. Thus, timers cannot be completely GCed. They must be explicitly removed from heap via deltimer [3]; Stop is essentially a wrapper for it. If Stop is never invoked, both memory and CPU resources end up leaking.

While the memory footprint of a single Ticker (or Timer) is constant and negligible, it may require a non-constant amount of CPU work to keep ticking. Consequently, a large number of leaked Tickers can result in a non-trivial increase of CPU utilization. (Example below illustrates a high CPU utilization.)

Preliminary Finding

A preliminary investigation using semgrep (rules attached) revealed potential Ticker and Timer leaks,

• t in chaosLoop
• ticker in jobSurvivesNodeShutdown
• timer in run
  • NB: control can escape before timer.Stop is invoked
• ticker in makeCSVReporter
• ticker in addToPurgatoryLocked
• t in heartbeatLoop
• timer in run
• timer in startResolver

NOTE: this is not the first time Ticker leak was identified [4]. Hence, as the codebase continues to evolve, we should attempt to catch it statically.

[1] https://pkg.go.dev/time#NewTicker [2] https://github.com/golang/go/issues/8001 [3] https://github.com/golang/go/blob/master/src/runtime/time.go#L314 [4] https://github.com/cockroachdb/cockroach/pull/1201

Jira issue: CRDB-20387

[blathers-crl[bot]]

cc @cockroachdb/test-eng

[srosenberg]

semgrep was invoked via the following,

NO_COLOR=true semgrep --verbose --config ~/.semgrep/rules/missing_stop.yml pkg/ >~/missing_stop_out.txt

NOTE: rename the attached to missing_stop.yml.

missing_stop.txt

[srosenberg]

The example below [1] is based on [2]. It first allocates 100,000 Tickers, one per goroutine. Subsequently, each goroutine exits while the tickers leak. The result is high cpu utilization,

go run leaking_timer.go
Making 100000 tickers
Total heap: 48111384
Sleeping... check CPU utilization

PID    COMMAND      %CPU
88757  leaking_timer 131.6

Rerunning the above after uncommenting defer t.Stop() yields,

PID    COMMAND       %CPU TIME
89379  leaking_timer 1.7

[1] https://gist.github.com/srosenberg/a4b920560f61c1f8a41869dfaed051bc [2] https://github.com/golang/go/issues/11662#issue-94398002

[github-actions[bot]]

We have marked this issue as stale because it has been inactive for 18 months. If this issue is still relevant, removing the stale label or adding a comment will keep it active. Otherwise, we'll close it in 10 days to keep the issue queue tidy. Thank you for your contribution to CockroachDB!