From 52ce4c20f8bc9b6da5fc1274bcce27c0b9dd738a Mon Sep 17 00:00:00 2001
From: Anton Kaliaev <anton.kalyaev@gmail.com>
Date: Fri, 9 Feb 2018 13:31:32 +0400
Subject: [PATCH] Fix RepeatTimer memory leak (#137)

fix RepeatTimer memory leak (Refs #137)

* test case

* drain channels on reset

Leaking memory:
```
leaktest.go:144: leaktest: leaked goroutine: goroutine 116 [chan send]:
        github.com/tendermint/tmlibs/common.(*RepeatTimer).fireRoutine(0xc42006a410, 0xc4203403c0, 0xc42031b2c0)
                /go/src/github.com/tendermint/tmlibs/common/repeat_timer.go:160 +0x6e
        created by github.com/tendermint/tmlibs/common.(*RepeatTimer).reset
                /go/src/github.com/tendermint/tmlibs/common/repeat_timer.go:196 +0xe9
```

The alternative solution could be draining channels on the client side.

* add one more select instead of draining

thanks to Jae
---
 common/repeat_timer.go      |  7 +++++--
 common/repeat_timer_test.go | 33 +++++++++++++++++++++++++++++++++
 glide.lock                  |  2 ++
 glide.yaml                  |  1 +
 4 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/common/repeat_timer.go b/common/repeat_timer.go
index cb227199..dba5fbad 100644
--- a/common/repeat_timer.go
+++ b/common/repeat_timer.go
@@ -155,7 +155,11 @@ func (t *RepeatTimer) fireRoutine(ch <-chan time.Time, quit <-chan struct{}) {
 	for {
 		select {
 		case t_ := <-ch:
-			t.ch <- t_
+			select {
+			case t.ch <- t_:
+			case <-quit:
+				return
+			}
 		case <-quit: // NOTE: `t.quit` races.
 			return
 		}
@@ -210,7 +214,6 @@ func (t *RepeatTimer) stop() {
 	t.ticker.Stop()
 	t.ticker = nil
 	/*
-		XXX
 		From https://golang.org/pkg/time/#Ticker:
 		"Stop the ticker to release associated resources"
 		"After Stop, no more ticks will be sent"
diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go
index 5598922c..160f4394 100644
--- a/common/repeat_timer_test.go
+++ b/common/repeat_timer_test.go
@@ -1,10 +1,12 @@
 package common
 
 import (
+	"math/rand"
 	"sync"
 	"testing"
 	"time"
 
+	"github.com/fortytw2/leaktest"
 	"github.com/stretchr/testify/assert"
 )
 
@@ -102,3 +104,34 @@ func TestRepeatTimer(t *testing.T) {
 	// Another stop panics.
 	assert.Panics(t, func() { rt.Stop() })
 }
+
+func TestRepeatTimerReset(t *testing.T) {
+	// check that we are not leaking any go-routines
+	defer leaktest.Check(t)()
+
+	timer := NewRepeatTimer("test", 20*time.Millisecond)
+	defer timer.Stop()
+
+	// test we don't receive tick before duration ms.
+	select {
+	case <-timer.Chan():
+		t.Fatal("did not expect to receive tick")
+	default:
+	}
+
+	timer.Reset()
+
+	// test we receive tick after Reset is called
+	select {
+	case <-timer.Chan():
+		// all good
+	case <-time.After(40 * time.Millisecond):
+		t.Fatal("expected to receive tick after reset")
+	}
+
+	// just random calls
+	for i := 0; i < 100; i++ {
+		time.Sleep(time.Duration(rand.Intn(40)) * time.Millisecond)
+		timer.Reset()
+	}
+}
diff --git a/glide.lock b/glide.lock
index 10dec980..a0ada5a4 100644
--- a/glide.lock
+++ b/glide.lock
@@ -95,6 +95,8 @@ testImports:
   version: 346938d642f2ec3594ed81d874461961cd0faa76
   subpackages:
   - spew
+- name: github.com/fortytw2/leaktest
+  version: 3b724c3d7b8729a35bf4e577f71653aec6e53513
 - name: github.com/pmezard/go-difflib
   version: d8ed2627bdf02c080bf22230dbb337003b7aba2d
   subpackages:
diff --git a/glide.yaml b/glide.yaml
index b12c72a1..cf3da346 100644
--- a/glide.yaml
+++ b/glide.yaml
@@ -35,3 +35,4 @@ testImport:
   subpackages:
   - assert
   - require
+- package: github.com/fortytw2/leaktest