Merge pull request #75 from tendermint/45-change-common-start-signature

change common start signature

Makefile

@@ -4,14 +4,16 @@ GOTOOLS = \
 	github.com/Masterminds/glide \
 	github.com/alecthomas/gometalinter
 
+PACKAGES=$(shell go list ./... | grep -v '/vendor/')
 REPO:=github.com/tendermint/tmlibs
 
 all: test
 
-NOVENDOR = go list github.com/tendermint/tmlibs/... | grep -v /vendor/
-
 test:
-	go test `glide novendor`
+	@echo "--> Running linter"
+	@make metalinter_test
+	@echo "--> Running go test"
+	@go test $(PACKAGES)
 
 get_vendor_deps: ensure_tools
 	@rm -rf vendor/
@@ -20,16 +22,14 @@ get_vendor_deps: ensure_tools
 
 ensure_tools:
 	go get $(GOTOOLS)
-
-metalinter: ensure_tools
 	@gometalinter --install
+
+metalinter: 
 	gometalinter --vendor --deadline=600s --enable-all --disable=lll ./...
 
-metalinter_test: ensure_tools
-	@gometalinter --install
+metalinter_test: 
 	gometalinter --vendor --deadline=600s --disable-all  \
 		--enable=deadcode \
-		--enable=gas \
 		--enable=goconst \
 		--enable=gosimple \
 	 	--enable=ineffassign \
@@ -46,6 +46,7 @@ metalinter_test: ensure_tools
 		--enable=vet \
 		./...
 
+		#--enable=gas \
 		#--enable=aligncheck \
 		#--enable=dupl \
 		#--enable=errcheck \

circle.yml

@@ -15,7 +15,7 @@ dependencies:
 
 test:
   override:
-    - cd $PROJECT_PATH && make get_vendor_deps && make metalinter_test && bash ./test.sh
+    - cd $PROJECT_PATH && make get_vendor_deps && bash ./test.sh
   post:
     - cd "$PROJECT_PATH" && bash <(curl -s https://codecov.io/bash) -f coverage.txt
     - cd "$PROJECT_PATH" && mv coverage.txt "${CIRCLE_ARTIFACTS}"

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)
 }

events/events_test.go

@@ -13,8 +13,8 @@ import (
 // listener to an event, and sends a string "data".
 func TestAddListenerForEventFireOnce(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	messages := make(chan EventData)
@@ -33,8 +33,8 @@ func TestAddListenerForEventFireOnce(t *testing.T) {
 // listener to an event, and sends a thousand integers.
 func TestAddListenerForEventFireMany(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	doneSum := make(chan uint64)
@@ -62,8 +62,8 @@ func TestAddListenerForEventFireMany(t *testing.T) {
 // of the three events.
 func TestAddListenerForDifferentEvents(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	doneSum := make(chan uint64)
@@ -107,8 +107,8 @@ func TestAddListenerForDifferentEvents(t *testing.T) {
 // for each of the three events.
 func TestAddDifferentListenerForDifferentEvents(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	doneSum1 := make(chan uint64)
@@ -167,8 +167,8 @@ func TestAddDifferentListenerForDifferentEvents(t *testing.T) {
 // the listener and fires a thousand integers for the second event.
 func TestAddAndRemoveListener(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	doneSum1 := make(chan uint64)
@@ -212,8 +212,8 @@ func TestAddAndRemoveListener(t *testing.T) {
 // TestRemoveListener does basic tests on adding and removing
 func TestRemoveListener(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	count := 10
@@ -265,8 +265,8 @@ func TestRemoveListener(t *testing.T) {
 // `go test -race`, to examine for possible race conditions.
 func TestRemoveListenersAsync(t *testing.T) {
 	evsw := NewEventSwitch()
-	started, err := evsw.Start()
-	if !started || err != nil {
+	err := evsw.Start()
+	if err != nil {
 		t.Errorf("Failed to start EventSwitch, error: %v", err)
 	}
 	doneSum1 := make(chan uint64)

test.sh

@@ -1,8 +1,11 @@
 #!/usr/bin/env bash
-
 set -e
-echo "" > coverage.txt
 
+# run the linter
+make metalinter_test
+
+# run the unit tests with coverage
+echo "" > coverage.txt
 for d in $(go list ./... | grep -v vendor); do
     go test -race -coverprofile=profile.out -covermode=atomic "$d"
     if [ -f profile.out ]; then