Skip to content

Commit

Permalink
Prevent timer leaks in Workerpool and others (#11333) (#11340)
Browse files Browse the repository at this point in the history
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 <[email protected]>

Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>

Co-authored-by: techknowlogick <[email protected]>
Co-authored-by: Lunny Xiao <[email protected]>
  • Loading branch information
3 people authored May 8, 2020
1 parent f3a9005 commit 86863ae
Show file tree
Hide file tree
Showing 5 changed files with 32 additions and 29 deletions.
12 changes: 3 additions & 9 deletions cmd/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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())
Expand Down
3 changes: 2 additions & 1 deletion modules/migrations/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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:
}
Expand Down
3 changes: 2 additions & 1 deletion modules/queue/queue_wrapped.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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:
}
}
Expand Down
22 changes: 4 additions & 18 deletions modules/queue/workerpool.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"time"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/util"
)

// WorkerPool takes
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -277,25 +273,15 @@ 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...)
}
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 {
Expand Down
21 changes: 21 additions & 0 deletions modules/util/timer.go
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit 86863ae

Please sign in to comment.