Merge branch 'develop' into sdk2

common/bit_array.go

@@ -3,7 +3,6 @@ package common
 import (
 	"encoding/binary"
 	"fmt"
-	"math/rand"
 	"strings"
 	"sync"
 )
@@ -212,12 +211,12 @@ func (bA *BitArray) PickRandom() (int, bool) {
 	if length == 0 {
 		return 0, false
 	}
-	randElemStart := rand.Intn(length)
+	randElemStart := RandIntn(length)
 	for i := 0; i < length; i++ {
 		elemIdx := ((i + randElemStart) % length)
 		if elemIdx < length-1 {
 			if bA.Elems[elemIdx] > 0 {
-				randBitStart := rand.Intn(64)
+				randBitStart := RandIntn(64)
 				for j := 0; j < 64; j++ {
 					bitIdx := ((j + randBitStart) % 64)
 					if (bA.Elems[elemIdx] & (uint64(1) << uint(bitIdx))) > 0 {
@@ -232,7 +231,7 @@ func (bA *BitArray) PickRandom() (int, bool) {
 			if elemBits == 0 {
 				elemBits = 64
 			}
-			randBitStart := rand.Intn(elemBits)
+			randBitStart := RandIntn(elemBits)
 			for j := 0; j < elemBits; j++ {
 				bitIdx := ((j + randBitStart) % elemBits)
 				if (bA.Elems[elemIdx] & (uint64(1) << uint(bitIdx))) > 0 {

common/int.go

@@ -53,3 +53,13 @@ func PutInt64BE(dest []byte, i int64) {
 func GetInt64BE(src []byte) int64 {
 	return int64(binary.BigEndian.Uint64(src))
 }
+
+// IntInSlice returns true if a is found in the list.
+func IntInSlice(a int, list []int) bool {
+	for _, b := range list {
+		if b == a {
+			return true
+		}
+	}
+	return false
+}

common/int_test.go

@@ -0,0 +1,14 @@
+package common
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIntInSlice(t *testing.T) {
+	assert.True(t, IntInSlice(1, []int{1, 2, 3}))
+	assert.False(t, IntInSlice(4, []int{1, 2, 3}))
+	assert.True(t, IntInSlice(0, []int{0}))
+	assert.False(t, IntInSlice(0, []int{}))
+}

common/random.go

@@ -2,7 +2,8 @@ package common
 
 import (
 	crand "crypto/rand"
-	"math/rand"
+	mrand "math/rand"
+	"sync"
 	"time"
 )
 
@@ -10,22 +11,36 @@ const (
 	strChars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" // 62 characters
 )
 
-func init() {
+// pseudo random number generator.
+// seeded with OS randomness (crand)
+var prng struct {
+	sync.Mutex
+	*mrand.Rand
+}
+
+func reset() {
 	b := cRandBytes(8)
 	var seed uint64
 	for i := 0; i < 8; i++ {
 		seed |= uint64(b[i])
 		seed <<= 8
 	}
-	rand.Seed(int64(seed))
+	prng.Lock()
+	prng.Rand = mrand.New(mrand.NewSource(int64(seed)))
+	prng.Unlock()
+}
+
+func init() {
+	reset()
 }
 
 // Constructs an alphanumeric string of given length.
+// It is not safe for cryptographic usage.
 func RandStr(length int) string {
 	chars := []byte{}
 MAIN_LOOP:
 	for {
-		val := rand.Int63()
+		val := RandInt63()
 		for i := 0; i < 10; i++ {
 			v := int(val & 0x3f) // rightmost 6 bits
 			if v >= 62 {         // only 62 characters in strChars
@@ -44,87 +59,151 @@ MAIN_LOOP:
 	return string(chars)
 }
 
+// It is not safe for cryptographic usage.
 func RandUint16() uint16 {
-	return uint16(rand.Uint32() & (1<<16 - 1))
+	return uint16(RandUint32() & (1<<16 - 1))
 }
 
+// It is not safe for cryptographic usage.
 func RandUint32() uint32 {
-	return rand.Uint32()
+	prng.Lock()
+	u32 := prng.Uint32()
+	prng.Unlock()
+	return u32
 }
 
+// It is not safe for cryptographic usage.
 func RandUint64() uint64 {
-	return uint64(rand.Uint32())<<32 + uint64(rand.Uint32())
+	return uint64(RandUint32())<<32 + uint64(RandUint32())
 }
 
+// It is not safe for cryptographic usage.
 func RandUint() uint {
-	return uint(rand.Int())
+	prng.Lock()
+	i := prng.Int()
+	prng.Unlock()
+	return uint(i)
 }
 
+// It is not safe for cryptographic usage.
 func RandInt16() int16 {
-	return int16(rand.Uint32() & (1<<16 - 1))
+	return int16(RandUint32() & (1<<16 - 1))
 }
 
+// It is not safe for cryptographic usage.
 func RandInt32() int32 {
-	return int32(rand.Uint32())
+	return int32(RandUint32())
 }
 
+// It is not safe for cryptographic usage.
 func RandInt64() int64 {
-	return int64(rand.Uint32())<<32 + int64(rand.Uint32())
+	return int64(RandUint64())
 }
 
+// It is not safe for cryptographic usage.
 func RandInt() int {
-	return rand.Int()
+	prng.Lock()
+	i := prng.Int()
+	prng.Unlock()
+	return i
+}
+
+// It is not safe for cryptographic usage.
+func RandInt31() int32 {
+	prng.Lock()
+	i31 := prng.Int31()
+	prng.Unlock()
+	return i31
+}
+
+// It is not safe for cryptographic usage.
+func RandInt63() int64 {
+	prng.Lock()
+	i63 := prng.Int63()
+	prng.Unlock()
+	return i63
 }
 
 // Distributed pseudo-exponentially to test for various cases
+// It is not safe for cryptographic usage.
 func RandUint16Exp() uint16 {
-	bits := rand.Uint32() % 16
+	bits := RandUint32() % 16
 	if bits == 0 {
 		return 0
 	}
 	n := uint16(1 << (bits - 1))
-	n += uint16(rand.Int31()) & ((1 << (bits - 1)) - 1)
+	n += uint16(RandInt31()) & ((1 << (bits - 1)) - 1)
 	return n
 }
 
 // Distributed pseudo-exponentially to test for various cases
+// It is not safe for cryptographic usage.
 func RandUint32Exp() uint32 {
-	bits := rand.Uint32() % 32
+	bits := RandUint32() % 32
 	if bits == 0 {
 		return 0
 	}
 	n := uint32(1 << (bits - 1))
-	n += uint32(rand.Int31()) & ((1 << (bits - 1)) - 1)
+	n += uint32(RandInt31()) & ((1 << (bits - 1)) - 1)
 	return n
 }
 
 // Distributed pseudo-exponentially to test for various cases
+// It is not safe for cryptographic usage.
 func RandUint64Exp() uint64 {
-	bits := rand.Uint32() % 64
+	bits := RandUint32() % 64
 	if bits == 0 {
 		return 0
 	}
 	n := uint64(1 << (bits - 1))
-	n += uint64(rand.Int63()) & ((1 << (bits - 1)) - 1)
+	n += uint64(RandInt63()) & ((1 << (bits - 1)) - 1)
 	return n
 }
 
+// It is not safe for cryptographic usage.
 func RandFloat32() float32 {
-	return rand.Float32()
+	prng.Lock()
+	f32 := prng.Float32()
+	prng.Unlock()
+	return f32
 }
 
+// It is not safe for cryptographic usage.
 func RandTime() time.Time {
 	return time.Unix(int64(RandUint64Exp()), 0)
 }
 
+// RandBytes returns n random bytes from the OS's source of entropy ie. via crypto/rand.
+// It is not safe for cryptographic usage.
 func RandBytes(n int) []byte {
+	// cRandBytes isn't guaranteed to be fast so instead
+	// use random bytes generated from the internal PRNG
 	bs := make([]byte, n)
-	for i := 0; i < n; i++ {
-		bs[i] = byte(rand.Intn(256))
+	for i := 0; i < len(bs); i++ {
+		bs[i] = byte(RandInt() & 0xFF)
 	}
 	return bs
 }
 
+// RandIntn returns, as an int, a non-negative pseudo-random number in [0, n).
+// It panics if n <= 0.
+// It is not safe for cryptographic usage.
+func RandIntn(n int) int {
+	prng.Lock()
+	i := prng.Intn(n)
+	prng.Unlock()
+	return i
+}
+
+// RandPerm returns a pseudo-random permutation of n integers in [0, n).
+// It is not safe for cryptographic usage.
+func RandPerm(n int) []int {
+	prng.Lock()
+	perm := prng.Perm(n)
+	prng.Unlock()
+	return perm
+}
+
 // NOTE: This relies on the os's random number generator.
 // For real security, we should salt that with some seed.
 // See github.com/tendermint/go-crypto for a more secure reader.

common/random_test.go

@@ -0,0 +1,120 @@
+package common
+
+import (
+	"bytes"
+	"encoding/json"
+	"fmt"
+	"io"
+	mrand "math/rand"
+	"sync"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestRandStr(t *testing.T) {
+	l := 243
+	s := RandStr(l)
+	assert.Equal(t, l, len(s))
+}
+
+func TestRandBytes(t *testing.T) {
+	l := 243
+	b := RandBytes(l)
+	assert.Equal(t, l, len(b))
+}
+
+func TestRandIntn(t *testing.T) {
+	n := 243
+	for i := 0; i < 100; i++ {
+		x := RandIntn(n)
+		assert.True(t, x < n)
+	}
+}
+
+// It is essential that these tests run and never repeat their outputs
+// lest we've been pwned and the behavior of our randomness is controlled.
+// See Issues:
+//  * https://github.com/tendermint/tmlibs/issues/99
+//  * https://github.com/tendermint/tendermint/issues/973
+func TestUniqueRng(t *testing.T) {
+	buf := new(bytes.Buffer)
+	outputs := make(map[string][]int)
+	for i := 0; i < 100; i++ {
+		testThemAll(buf)
+		output := buf.String()
+		buf.Reset()
+		runs, seen := outputs[output]
+		if seen {
+			t.Errorf("Run #%d's output was already seen in previous runs: %v", i, runs)
+		}
+		outputs[output] = append(outputs[output], i)
+	}
+}
+
+func testThemAll(out io.Writer) {
+	// Reset the internal PRNG
+	reset()
+
+	// Set math/rand's Seed so that any direct invocations
+	// of math/rand will reveal themselves.
+	mrand.Seed(1)
+	perm := RandPerm(10)
+	blob, _ := json.Marshal(perm)
+	fmt.Fprintf(out, "perm: %s\n", blob)
+
+	fmt.Fprintf(out, "randInt: %d\n", RandInt())
+	fmt.Fprintf(out, "randUint: %d\n", RandUint())
+	fmt.Fprintf(out, "randIntn: %d\n", RandIntn(97))
+	fmt.Fprintf(out, "randInt31: %d\n", RandInt31())
+	fmt.Fprintf(out, "randInt32: %d\n", RandInt32())
+	fmt.Fprintf(out, "randInt63: %d\n", RandInt63())
+	fmt.Fprintf(out, "randInt64: %d\n", RandInt64())
+	fmt.Fprintf(out, "randUint32: %d\n", RandUint32())
+	fmt.Fprintf(out, "randUint64: %d\n", RandUint64())
+	fmt.Fprintf(out, "randUint16Exp: %d\n", RandUint16Exp())
+	fmt.Fprintf(out, "randUint32Exp: %d\n", RandUint32Exp())
+	fmt.Fprintf(out, "randUint64Exp: %d\n", RandUint64Exp())
+}
+
+func TestRngConcurrencySafety(t *testing.T) {
+	var wg sync.WaitGroup
+	for i := 0; i < 100; i++ {
+		wg.Add(1)
+		go func() {
+			defer wg.Done()
+
+			_ = RandUint64()
+			<-time.After(time.Millisecond * time.Duration(RandIntn(100)))
+			_ = RandPerm(3)
+		}()
+	}
+	wg.Wait()
+}
+
+func BenchmarkRandBytes10B(b *testing.B) {
+	benchmarkRandBytes(b, 10)
+}
+func BenchmarkRandBytes100B(b *testing.B) {
+	benchmarkRandBytes(b, 100)
+}
+func BenchmarkRandBytes1KiB(b *testing.B) {
+	benchmarkRandBytes(b, 1024)
+}
+func BenchmarkRandBytes10KiB(b *testing.B) {
+	benchmarkRandBytes(b, 10*1024)
+}
+func BenchmarkRandBytes100KiB(b *testing.B) {
+	benchmarkRandBytes(b, 100*1024)
+}
+func BenchmarkRandBytes1MiB(b *testing.B) {
+	benchmarkRandBytes(b, 1024*1024)
+}
+
+func benchmarkRandBytes(b *testing.B, n int) {
+	for i := 0; i < b.N; i++ {
+		_ = RandBytes(n)
+	}
+	b.ReportAllocs()
+}

common/repeat_timer.go

@@ -5,82 +5,224 @@ import (
 	"time"
 )
 
-/*
-RepeatTimer repeatedly sends a struct{}{} to .Ch after each "dur" period.
-It's good for keeping connections alive.
-A RepeatTimer must be Stop()'d or it will keep a goroutine alive.
-*/
-type RepeatTimer struct {
-	Ch chan time.Time
+// Used by RepeatTimer the first time,
+// and every time it's Reset() after Stop().
+type TickerMaker func(dur time.Duration) Ticker
 
-	mtx    sync.Mutex
-	name   string
-	ticker *time.Ticker
-	quit   chan struct{}
-	wg     *sync.WaitGroup
-	dur    time.Duration
+// Ticker is a basic ticker interface.
+type Ticker interface {
+
+	// Never changes, never closes.
+	Chan() <-chan time.Time
+
+	// Stopping a stopped Ticker will panic.
+	Stop()
 }
 
-func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer {
-	var t = &RepeatTimer{
-		Ch:     make(chan time.Time),
-		ticker: time.NewTicker(dur),
-		quit:   make(chan struct{}),
-		wg:     new(sync.WaitGroup),
-		name:   name,
-		dur:    dur,
+//----------------------------------------
+// defaultTickerMaker
+
+func defaultTickerMaker(dur time.Duration) Ticker {
+	ticker := time.NewTicker(dur)
+	return (*defaultTicker)(ticker)
+}
+
+type defaultTicker time.Ticker
+
+// Implements Ticker
+func (t *defaultTicker) Chan() <-chan time.Time {
+	return t.C
+}
+
+// Implements Ticker
+func (t *defaultTicker) Stop() {
+	((*time.Ticker)(t)).Stop()
+}
+
+//----------------------------------------
+// LogicalTickerMaker
+
+// Construct a TickerMaker that always uses `source`.
+// It's useful for simulating a deterministic clock.
+func NewLogicalTickerMaker(source chan time.Time) TickerMaker {
+	return func(dur time.Duration) Ticker {
+		return newLogicalTicker(source, dur)
 	}
-	t.wg.Add(1)
-	go t.fireRoutine(t.ticker)
+}
+
+type logicalTicker struct {
+	source <-chan time.Time
+	ch     chan time.Time
+	quit   chan struct{}
+}
+
+func newLogicalTicker(source <-chan time.Time, interval time.Duration) Ticker {
+	lt := &logicalTicker{
+		source: source,
+		ch:     make(chan time.Time),
+		quit:   make(chan struct{}),
+	}
+	go lt.fireRoutine(interval)
+	return lt
+}
+
+// We need a goroutine to read times from t.source
+// and fire on t.Chan() when `interval` has passed.
+func (t *logicalTicker) fireRoutine(interval time.Duration) {
+	source := t.source
+
+	// Init `lasttime`
+	lasttime := time.Time{}
+	select {
+	case lasttime = <-source:
+	case <-t.quit:
+		return
+	}
+	// Init `lasttime` end
+
+	timeleft := interval
+	for {
+		select {
+		case newtime := <-source:
+			elapsed := newtime.Sub(lasttime)
+			timeleft -= elapsed
+			if timeleft <= 0 {
+				// Block for determinism until the ticker is stopped.
+				select {
+				case t.ch <- newtime:
+				case <-t.quit:
+					return
+				}
+				// Reset timeleft.
+				// Don't try to "catch up" by sending more.
+				// "Ticker adjusts the intervals or drops ticks to make up for
+				// slow receivers" - https://golang.org/pkg/time/#Ticker
+				timeleft = interval
+			}
+		case <-t.quit:
+			return // done
+		}
+	}
+}
+
+// Implements Ticker
+func (t *logicalTicker) Chan() <-chan time.Time {
+	return t.ch // immutable
+}
+
+// Implements Ticker
+func (t *logicalTicker) Stop() {
+	close(t.quit) // it *should* panic when stopped twice.
+}
+
+//---------------------------------------------------------------------
+
+/*
+	RepeatTimer repeatedly sends a struct{}{} to `.Chan()` after each `dur`
+	period. (It's good for keeping connections alive.)
+	A RepeatTimer must be stopped, or it will keep a goroutine alive.
+*/
+type RepeatTimer struct {
+	name string
+	ch   chan time.Time
+	tm   TickerMaker
+
+	mtx    sync.Mutex
+	dur    time.Duration
+	ticker Ticker
+	quit   chan struct{}
+}
+
+// NewRepeatTimer returns a RepeatTimer with a defaultTicker.
+func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer {
+	return NewRepeatTimerWithTickerMaker(name, dur, defaultTickerMaker)
+}
+
+// NewRepeatTimerWithTicker returns a RepeatTimer with the given ticker
+// maker.
+func NewRepeatTimerWithTickerMaker(name string, dur time.Duration, tm TickerMaker) *RepeatTimer {
+	var t = &RepeatTimer{
+		name:   name,
+		ch:     make(chan time.Time),
+		tm:     tm,
+		dur:    dur,
+		ticker: nil,
+		quit:   nil,
+	}
+	t.reset()
 	return t
 }
 
-func (t *RepeatTimer) fireRoutine(ticker *time.Ticker) {
+func (t *RepeatTimer) fireRoutine(ch <-chan time.Time, quit <-chan struct{}) {
 	for {
 		select {
-		case t_ := <-ticker.C:
-			t.Ch <- t_
-		case <-t.quit:
-			// needed so we know when we can reset t.quit
-			t.wg.Done()
+		case t_ := <-ch:
+			t.ch <- t_
+		case <-quit: // NOTE: `t.quit` races.
 			return
 		}
 	}
 }
 
-// Wait the duration again before firing.
-func (t *RepeatTimer) Reset() {
-	t.Stop()
-
-	t.mtx.Lock() // Lock
-	defer t.mtx.Unlock()
-
-	t.ticker = time.NewTicker(t.dur)
-	t.quit = make(chan struct{})
-	t.wg.Add(1)
-	go t.fireRoutine(t.ticker)
+func (t *RepeatTimer) Chan() <-chan time.Time {
+	return t.ch
 }
 
-// For ease of .Stop()'ing services before .Start()'ing them,
-// we ignore .Stop()'s on nil RepeatTimers.
-func (t *RepeatTimer) Stop() bool {
-	if t == nil {
-		return false
-	}
-	t.mtx.Lock() // Lock
+func (t *RepeatTimer) Stop() {
+	t.mtx.Lock()
 	defer t.mtx.Unlock()
 
-	exists := t.ticker != nil
-	if exists {
-		t.ticker.Stop() // does not close the channel
+	t.stop()
+}
+
+// Wait the duration again before firing.
+func (t *RepeatTimer) Reset() {
+	t.mtx.Lock()
+	defer t.mtx.Unlock()
+
+	t.reset()
+}
+
+//----------------------------------------
+// Misc.
+
+// CONTRACT: (non-constructor) caller should hold t.mtx.
+func (t *RepeatTimer) reset() {
+	if t.ticker != nil {
+		t.stop()
+	}
+	t.ticker = t.tm(t.dur)
+	t.quit = make(chan struct{})
+	go t.fireRoutine(t.ticker.Chan(), t.quit)
+}
+
+// CONTRACT: caller should hold t.mtx.
+func (t *RepeatTimer) stop() {
+	if t.ticker == nil {
+		/*
+			Similar to the case of closing channels twice:
+			https://groups.google.com/forum/#!topic/golang-nuts/rhxMiNmRAPk
+			Stopping a RepeatTimer twice implies that you do
+			not know whether you are done or not.
+			If you're calling stop on a stopped RepeatTimer,
+			you probably have race conditions.
+		*/
+		panic("Tried to stop a stopped RepeatTimer")
+	}
+	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"
+		So we shouldn't have to do the below.
+
 		select {
-		case <-t.Ch:
+		case <-t.ch:
 			// read off channel if there's anything there
 		default:
 		}
-		close(t.quit)
-		t.wg.Wait() // must wait for quit to close else we race Reset
-		t.ticker = nil
-	}
-	return exists
+	*/
+	close(t.quit)
 }

common/repeat_timer_test.go

@@ -0,0 +1,92 @@
+package common
+
+import (
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestDefaultTicker(t *testing.T) {
+	ticker := defaultTickerMaker(time.Millisecond * 10)
+	<-ticker.Chan()
+	ticker.Stop()
+}
+
+func TestRepeat(t *testing.T) {
+
+	ch := make(chan time.Time, 100)
+	lt := time.Time{} // zero time is year 1
+
+	// tick fires `cnt` times for each second.
+	tick := func(cnt int) {
+		for i := 0; i < cnt; i++ {
+			lt = lt.Add(time.Second)
+			ch <- lt
+		}
+	}
+
+	// tock consumes Ticker.Chan() events `cnt` times.
+	tock := func(t *testing.T, rt *RepeatTimer, cnt int) {
+		for i := 0; i < cnt; i++ {
+			timeout := time.After(time.Second * 10)
+			select {
+			case <-rt.Chan():
+			case <-timeout:
+				panic("expected RepeatTimer to fire")
+			}
+		}
+		done := true
+		select {
+		case <-rt.Chan():
+			done = false
+		default:
+		}
+		assert.True(t, done)
+	}
+
+	tm := NewLogicalTickerMaker(ch)
+	dur := time.Duration(10 * time.Millisecond) // less than a second
+	rt := NewRepeatTimerWithTickerMaker("bar", dur, tm)
+
+	// Start at 0.
+	tock(t, rt, 0)
+	tick(1) // init time
+
+	tock(t, rt, 0)
+	tick(1) // wait 1 periods
+	tock(t, rt, 1)
+	tick(2) // wait 2 periods
+	tock(t, rt, 2)
+	tick(3) // wait 3 periods
+	tock(t, rt, 3)
+	tick(4) // wait 4 periods
+	tock(t, rt, 4)
+
+	// Multiple resets leads to no firing.
+	for i := 0; i < 20; i++ {
+		time.Sleep(time.Millisecond)
+		rt.Reset()
+	}
+
+	// After this, it works as new.
+	tock(t, rt, 0)
+	tick(1) // init time
+
+	tock(t, rt, 0)
+	tick(1) // wait 1 periods
+	tock(t, rt, 1)
+	tick(2) // wait 2 periods
+	tock(t, rt, 2)
+	tick(3) // wait 3 periods
+	tock(t, rt, 3)
+	tick(4) // wait 4 periods
+	tock(t, rt, 4)
+
+	// After a stop, nothing more is sent.
+	rt.Stop()
+	tock(t, rt, 0)
+
+	// Another stop panics.
+	assert.Panics(t, func() { rt.Stop() })
+}

common/service.go

@@ -1,23 +1,41 @@
 package common
 
 import (
+	"errors"
+	"fmt"
 	"sync/atomic"
 
 	"github.com/tendermint/tmlibs/log"
 )
 
+var (
+	ErrAlreadyStarted = errors.New("already started")
+	ErrAlreadyStopped = errors.New("already stopped")
+)
+
+// Service defines a service that can be started, stopped, and reset.
 type Service interface {
-	Start() (bool, error)
+	// Start the service.
+	// If it's already started or stopped, will return an error.
+	// If OnStart() returns an error, it's returned by Start()
+	Start() error
 	OnStart() error
 
-	Stop() bool
+	// Stop the service.
+	// If it's already stopped, will return an error.
+	// OnStop must never error.
+	Stop() error
 	OnStop()
 
-	Reset() (bool, error)
+	// Reset the service.
+	// Panics by default - must be overwritten to enable reset.
+	Reset() error
 	OnReset() error
 
+	// Return true if the service is running
 	IsRunning() bool
 
+	// String representation of the service
 	String() string
 
 	SetLogger(log.Logger)
@@ -94,11 +112,11 @@ func (bs *BaseService) SetLogger(l log.Logger) {
 }
 
 // Implements Servce
-func (bs *BaseService) Start() (bool, error) {
+func (bs *BaseService) Start() error {
 	if atomic.CompareAndSwapUint32(&bs.started, 0, 1) {
 		if atomic.LoadUint32(&bs.stopped) == 1 {
 			bs.Logger.Error(Fmt("Not starting %v -- already stopped", bs.name), "impl", bs.impl)
-			return false, nil
+			return ErrAlreadyStopped
 		} else {
 			bs.Logger.Info(Fmt("Starting %v", bs.name), "impl", bs.impl)
 		}
@@ -106,12 +124,12 @@ func (bs *BaseService) Start() (bool, error) {
 		if err != nil {
 			// revert flag
 			atomic.StoreUint32(&bs.started, 0)
-			return false, err
+			return err
 		}
-		return true, err
+		return nil
 	} else {
 		bs.Logger.Debug(Fmt("Not starting %v -- already started", bs.name), "impl", bs.impl)
-		return false, nil
+		return ErrAlreadyStarted
 	}
 }
 
@@ -121,15 +139,15 @@ func (bs *BaseService) Start() (bool, error) {
 func (bs *BaseService) OnStart() error { return nil }
 
 // Implements Service
-func (bs *BaseService) Stop() bool {
+func (bs *BaseService) Stop() error {
 	if atomic.CompareAndSwapUint32(&bs.stopped, 0, 1) {
 		bs.Logger.Info(Fmt("Stopping %v", bs.name), "impl", bs.impl)
 		bs.impl.OnStop()
 		close(bs.Quit)
-		return true
+		return nil
 	} else {
 		bs.Logger.Debug(Fmt("Stopping %v (ignoring: already stopped)", bs.name), "impl", bs.impl)
-		return false
+		return ErrAlreadyStopped
 	}
 }
 
@@ -139,17 +157,17 @@ func (bs *BaseService) Stop() bool {
 func (bs *BaseService) OnStop() {}
 
 // Implements Service
-func (bs *BaseService) Reset() (bool, error) {
+func (bs *BaseService) Reset() error {
 	if !atomic.CompareAndSwapUint32(&bs.stopped, 1, 0) {
 		bs.Logger.Debug(Fmt("Can't reset %v. Not stopped", bs.name), "impl", bs.impl)
-		return false, nil
+		return fmt.Errorf("can't reset running %s", bs.name)
 	}
 
 	// whether or not we've started, we can reset
 	atomic.CompareAndSwapUint32(&bs.started, 1, 0)
 
 	bs.Quit = make(chan struct{})
-	return true, bs.impl.OnReset()
+	return bs.impl.OnReset()
 }
 
 // Implements Service

common/service_test.go

@@ -2,23 +2,53 @@ package common
 
 import (
 	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
 )
 
-func TestBaseServiceWait(t *testing.T) {
+type testService struct {
+	BaseService
+}
 
-	type TestService struct {
-		BaseService
-	}
-	ts := &TestService{}
+func (testService) OnReset() error {
+	return nil
+}
+
+func TestBaseServiceWait(t *testing.T) {
+	ts := &testService{}
 	ts.BaseService = *NewBaseService(nil, "TestService", ts)
 	ts.Start()
 
+	waitFinished := make(chan struct{})
 	go func() {
-		ts.Stop()
+		ts.Wait()
+		waitFinished <- struct{}{}
 	}()
 
-	for i := 0; i < 10; i++ {
-		ts.Wait()
-	}
+	go ts.Stop()
 
+	select {
+	case <-waitFinished:
+		// all good
+	case <-time.After(100 * time.Millisecond):
+		t.Fatal("expected Wait() to finish within 100 ms.")
+	}
+}
+
+func TestBaseServiceReset(t *testing.T) {
+	ts := &testService{}
+	ts.BaseService = *NewBaseService(nil, "TestService", ts)
+	ts.Start()
+
+	err := ts.Reset()
+	require.Error(t, err, "expected cant reset service error")
+
+	ts.Stop()
+
+	err = ts.Reset()
+	require.NoError(t, err)
+
+	err = ts.Start()
+	require.NoError(t, err)
 }

common/string.go

@@ -43,3 +43,13 @@ func StripHex(s string) string {
 	}
 	return s
 }
+
+// StringInSlice returns true if a is found the list.
+func StringInSlice(a string, list []string) bool {
+	for _, b := range list {
+		if b == a {
+			return true
+		}
+	}
+	return false
+}

common/string_test.go

@@ -0,0 +1,14 @@
+package common
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestStringInSlice(t *testing.T) {
+	assert.True(t, StringInSlice("a", []string{"a", "b", "c"}))
+	assert.False(t, StringInSlice("d", []string{"a", "b", "c"}))
+	assert.True(t, StringInSlice("", []string{""}))
+	assert.False(t, StringInSlice("", []string{}))
+}

common/throttle_timer_test.go

@@ -0,0 +1,78 @@
+package common
+
+import (
+	"sync"
+	"testing"
+	"time"
+
+	// make govet noshadow happy...
+	asrt "github.com/stretchr/testify/assert"
+)
+
+type thCounter struct {
+	input chan struct{}
+	mtx   sync.Mutex
+	count int
+}
+
+func (c *thCounter) Increment() {
+	c.mtx.Lock()
+	c.count++
+	c.mtx.Unlock()
+}
+
+func (c *thCounter) Count() int {
+	c.mtx.Lock()
+	val := c.count
+	c.mtx.Unlock()
+	return val
+}
+
+// Read should run in a go-routine and
+// updates count by one every time a packet comes in
+func (c *thCounter) Read() {
+	for range c.input {
+		c.Increment()
+	}
+}
+
+func TestThrottle(test *testing.T) {
+	assert := asrt.New(test)
+
+	ms := 50
+	delay := time.Duration(ms) * time.Millisecond
+	longwait := time.Duration(2) * delay
+	t := NewThrottleTimer("foo", delay)
+
+	// start at 0
+	c := &thCounter{input: t.Ch}
+	assert.Equal(0, c.Count())
+	go c.Read()
+
+	// waiting does nothing
+	time.Sleep(longwait)
+	assert.Equal(0, c.Count())
+
+	// send one event adds one
+	t.Set()
+	time.Sleep(longwait)
+	assert.Equal(1, c.Count())
+
+	// send a burst adds one
+	for i := 0; i < 5; i++ {
+		t.Set()
+	}
+	time.Sleep(longwait)
+	assert.Equal(2, c.Count())
+
+	// send 12, over 2 delay sections, adds 3
+	short := time.Duration(ms/5) * time.Millisecond
+	for i := 0; i < 13; i++ {
+		t.Set()
+		time.Sleep(short)
+	}
+	time.Sleep(longwait)
+	assert.Equal(5, c.Count())
+
+	close(t.Ch)
+}