cleanup: prevent leaks from time.After

[shoenig]

This PR replaces use of time.After with a safe helper function
that creates a time.Timer to use instead. The new function returns
both a time.Timer and a Stop function that the caller must handle.

Unlike time.NewTimer, the helper function does not panic if the duration
set is <= 0.

Fixes #11982

[shoenig]

helpers/ not available to api/, so no helper function here

• make sure we call t.Stop
• make sure the interval can never be non-positive

[tgross]

LGTM 👍

[tgross]

I really like the API on this. It makes it really obvious if we're accidentally misusing it by ex. calling NewSafeTimer inside the loop.

[lgfa29]

Nice helper 👍

I used this semgrep rule and it find some other instance of this pattern:

rules:
  - id: "time-after-leak"
    patterns:
      - pattern: |
          select {
          case <- time.After(...): ...
          }
    message: "Potential leak of timer"
    languages:
      - "go"
    severity: "WARNING"
    paths:
      exclude:
        - "testutil/*"
        - "*testing.go"
        - "*_test.go"

But I don't know if the are all valid or if there are any false positives.

[shoenig]

I think that's a valid rule @lgfa29 in theory you don't need the for to leak the goroutines; it'll happen each time the enclosing function is called. Want to open a PR? And have the message point them towards NewSafeTimer

[lgfa29]

I think that's a valid rule @lgfa29 in theory you don't need the for to leak the goroutines; it'll happen each time the enclosing function is called. Want to open a PR? And have the message point them towards NewSafeTimer

Yeah, good idea, I will update the message 👍

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.