From 86863ae9395b77163dda50eb3a141877eb95eb12 Mon Sep 17 00:00:00 2001 From: zeripath Date: Fri, 8 May 2020 17:18:39 +0100 Subject: [PATCH] Prevent timer leaks in Workerpool and others (#11333) (#11340) There is a potential memory leak in `Workerpool` due to the intricacies of `time.Timer` stopping. Whenever a `time.Timer` is `Stop`ped its channel must be cleared using a `select` if the result of the `Stop()` is `false`. Unfortunately in `Workerpool` these were checked the wrong way round. However, there were a few other places that were not being checked. Signed-off-by: Andrew Thornton Co-authored-by: techknowlogick Co-authored-by: Lunny Xiao Co-authored-by: techknowlogick Co-authored-by: Lunny Xiao --- cmd/hook.go | 12 +++--------- modules/migrations/github.go | 3 ++- modules/queue/queue_wrapped.go | 3 ++- modules/queue/workerpool.go | 22 ++++------------------ modules/util/timer.go | 21 +++++++++++++++++++++ 5 files changed, 32 insertions(+), 29 deletions(-) create mode 100644 modules/util/timer.go diff --git a/cmd/hook.go b/cmd/hook.go index 331f6a2d2d..fa932087fe 100644 --- a/cmd/hook.go +++ b/cmd/hook.go @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/modules/git" "code.gitea.io/gitea/modules/private" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" "github.com/urfave/cli" ) @@ -113,15 +114,8 @@ func (d *delayWriter) Close() error { if d == nil { return nil } - stopped := d.timer.Stop() - if stopped { - return nil - } - select { - case <-d.timer.C: - default: - } - if d.buf == nil { + stopped := util.StopTimer(d.timer) + if stopped || d.buf == nil { return nil } _, err := d.internal.Write(d.buf.Bytes()) diff --git a/modules/migrations/github.go b/modules/migrations/github.go index 52344d0174..0749edee4a 100644 --- a/modules/migrations/github.go +++ b/modules/migrations/github.go @@ -16,6 +16,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/migrations/base" "code.gitea.io/gitea/modules/structs" + "code.gitea.io/gitea/modules/util" "github.com/google/go-github/v24/github" "golang.org/x/oauth2" @@ -121,7 +122,7 @@ func (g *GithubDownloaderV3) sleep() { timer := time.NewTimer(time.Until(g.rate.Reset.Time)) select { case <-g.ctx.Done(): - timer.Stop() + util.StopTimer(timer) return case <-timer.C: } diff --git a/modules/queue/queue_wrapped.go b/modules/queue/queue_wrapped.go index c52e6e4673..535c11f2db 100644 --- a/modules/queue/queue_wrapped.go +++ b/modules/queue/queue_wrapped.go @@ -12,6 +12,7 @@ import ( "time" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" ) // WrappedQueueType is the type for a wrapped delayed starting queue @@ -77,7 +78,7 @@ func (q *delayedStarter) setInternal(atShutdown func(context.Context, func()), h t := time.NewTimer(sleepTime) select { case <-ctx.Done(): - t.Stop() + util.StopTimer(t) case <-t.C: } } diff --git a/modules/queue/workerpool.go b/modules/queue/workerpool.go index 11115b0b7c..a4148081cb 100644 --- a/modules/queue/workerpool.go +++ b/modules/queue/workerpool.go @@ -10,6 +10,7 @@ import ( "time" "code.gitea.io/gitea/modules/log" + "code.gitea.io/gitea/modules/util" ) // WorkerPool takes @@ -56,12 +57,7 @@ func (p *WorkerPool) pushBoost(data Data) { p.lock.Unlock() select { case p.dataChan <- data: - if timer.Stop() { - select { - case <-timer.C: - default: - } - } + util.StopTimer(timer) case <-timer.C: p.lock.Lock() if p.blockTimeout > ourTimeout || (p.numberOfWorkers > p.maxNumberOfWorkers && p.maxNumberOfWorkers >= 0) { @@ -277,12 +273,7 @@ func (p *WorkerPool) doWork(ctx context.Context) { timer := time.NewTimer(delay) select { case <-ctx.Done(): - if timer.Stop() { - select { - case <-timer.C: - default: - } - } + util.StopTimer(timer) if len(data) > 0 { log.Trace("Handling: %d data, %v", len(data), data) p.handle(data...) @@ -290,12 +281,7 @@ func (p *WorkerPool) doWork(ctx context.Context) { log.Trace("Worker shutting down") return case datum, ok := <-p.dataChan: - if timer.Stop() { - select { - case <-timer.C: - default: - } - } + util.StopTimer(timer) if !ok { // the dataChan has been closed - we should finish up: if len(data) > 0 { diff --git a/modules/util/timer.go b/modules/util/timer.go new file mode 100644 index 0000000000..b682f376d9 --- /dev/null +++ b/modules/util/timer.go @@ -0,0 +1,21 @@ +// Copyright 2020 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package util + +import ( + "time" +) + +// StopTimer is a utility function to safely stop a time.Timer and clean its channel +func StopTimer(t *time.Timer) bool { + stopped := t.Stop() + if !stopped { + select { + case <-t.C: + default: + } + } + return stopped +} -- 2.39.5