refactor: Remove time.After from any Loops

[nathanjsweet]

time.After is not garbage collected until after
its channel fires once. In situations where it is called
repeatedely this can lead to significant memory build up.
Instead of creating a new timer object for every iteration,
a single timer, with a hard-reset mechanism, is used in
situations where time.After is being called repeatedely.

Signed-off-by: Nate Sweet nathanjsweet@pm.me

Fixes: #14219

[brb]

Could we add a link to the article in the commit msg about time.After not being GC'd? It will help the future's readers.

[nathanjsweet]

test-me-please

[aditighag]

I'm not sure why we need a wrapper. Why isn't the in-place fix mentioned in the medium article sufficient?

[christarazi]

+1

[nathanjsweet]

The solution suggested in the article is actually partially incorrect. In order to reset a timer safely (as suggested in the article), you have to also stop the timer and clear the channel.
I think this wrapper is useful because it basically boils the following code:

t := time.NewTimer(time.Nanosecond)
defer t.Stop()
for {
...
        if !t.Stop() {
		// It could be that the channel was read already
		select {
		case <-t.C:
		default:
		}
	}
        t.Reset(someInterval)
        select {
        ...
        case <- t.C:
        }
}

to:

t, stop := inctimer.New()
defer stop()
for {
...
        select {
        ...
        case <- t.After(someInterval):
        }
}

In one case I believe the After function is called even more than once, so that would have either required the creation of two separate timers or the same hard reset logic repeated twice above the select.
I find this wrapper approach to be a lot more elegant and easier to use.

[christarazi]

The code looks good, but I'm also wondering the same as Aditi. Do we need a wrapper? If so, that would be helpful context in the commit msg and in the godoc of the inctimer package itself, for why it's necessary.

[christarazi]

+1

[christarazi]

Awesome, looks great! Thanks for adding the extra context. I think it makes sense to have the wrapper.

Thinking a bit into the future, I wonder how we can prevent uses of time.After without requiring code reviewers to remember. Help me understand, should we be replacing all uses of time.After with IncTimer going forward? If so, then we could do similar to what we have for lock.Mutex where we don't allow the use of the upstream sync.Mutex via a script (contrib/scripts/lock-check.sh) and this is enforced via our Golang GH Actions job.

[fristonio]

LGTM, thanks 🚀 Just one question/suggestion, does not need a second review.
Really cool work on the customvet check 🎉

[nathanjsweet]

test-me-please

[kkourt]

🚀

[tklauser]

This seems to have broken CI, see e.g. https://jenkins.cilium.io/job/cilium-ginkgo/job/cilium/job/master/6273

10:58:36      runtime: contrib/scripts/custom-vet-check.sh
10:58:36      runtime: # cd .; git clone -- https://github.com/cilium/customvet /home/vagrant/go/src/github.com/cilium/customvet
10:58:36      runtime: fatal: could not create work tree dir '/home/vagrant/go/src/github.com/cilium/customvet': Permission denied
10:58:36      runtime: package github.com/cilium/customvet: exit status 128
10:58:41      runtime: flag provided but not defined: -timeafter.ignore
10:58:41      runtime: usage: go vet [-n] [-x] [-vettool prog] [build flags] [vet flags] [packages]
10:58:41      runtime: Run 'go help vet' for details.
10:58:41      runtime: Run 'go tool vet -help' for the vet tool's flags.
10:58:41      runtime: Makefile:506: recipe for target 'precheck' failed
10:58:41      runtime: make: *** [precheck] Error 2

Looks like the CI runs failed on this PR already 😞

[tklauser]

I've sent a revert for this to unbreak CI: #14371