From 4123d54bf69cdd764bbc2b53ae545b4367267645 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 6 Nov 2017 12:18:04 -0500 Subject: [PATCH 1/5] change service#Start to return just error (Refs #45) ``` @melekes yeah, bool is superfluous @ethanfrey If I remember correctly when I was writing test code, if I call Start() on a Service that is already running, it returns (false, nil). Only if I try to legitimately start it, but it fails in startup do I get an error. The distinction is quite important to make it safe for reentrant calls. The other approach would be to have a special error type like ErrAlreadyStarted, then check for that in your code explicitly. Kind of like if I make a db call in gorm, and get an error, I check if it is a RecordNotFound error, or whether there was a real error with the db query. @melekes Ah, I see. Thanks. I must say I like ErrAlreadyStarted approach more (not just in Golang) ``` --- common/service.go | 18 ++++++++++++------ events/events_test.go | 28 ++++++++++++++-------------- 2 files changed, 26 insertions(+), 20 deletions(-) diff --git a/common/service.go b/common/service.go index 8d4de30a8..3973adab9 100644 --- a/common/service.go +++ b/common/service.go @@ -1,13 +1,19 @@ package common import ( + "errors" "sync/atomic" "github.com/tendermint/tmlibs/log" ) +var ( + ErrAlreadyStarted = errors.New("already started") + ErrAlreadyStopped = errors.New("already stopped") +) + type Service interface { - Start() (bool, error) + Start() error OnStart() error Stop() bool @@ -94,11 +100,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 +112,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 } } diff --git a/events/events_test.go b/events/events_test.go index dee50e5bd..87db2a304 100644 --- a/events/events_test.go +++ b/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) From e6164d40528b8621b01cacdb82efe72dee01eeb0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 6 Nov 2017 12:47:23 -0500 Subject: [PATCH 2/5] change service#Stop to be similar to Start --- common/service.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/service.go b/common/service.go index 3973adab9..32f531d1a 100644 --- a/common/service.go +++ b/common/service.go @@ -16,7 +16,7 @@ type Service interface { Start() error OnStart() error - Stop() bool + Stop() error OnStop() Reset() (bool, error) @@ -127,15 +127,15 @@ func (bs *BaseService) Start() 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 } } From c2fcc093b28e8c3c9ba99d0617127060c3c2e917 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 27 Nov 2017 23:42:36 -0600 Subject: [PATCH 3/5] remove bool from Service#Reset --- common/service.go | 9 +++++---- common/service_test.go | 46 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/common/service.go b/common/service.go index 32f531d1a..608f8b722 100644 --- a/common/service.go +++ b/common/service.go @@ -2,6 +2,7 @@ package common import ( "errors" + "fmt" "sync/atomic" "github.com/tendermint/tmlibs/log" @@ -19,7 +20,7 @@ type Service interface { Stop() error OnStop() - Reset() (bool, error) + Reset() error OnReset() error IsRunning() bool @@ -145,17 +146,17 @@ func (bs *BaseService) Stop() error { 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 diff --git a/common/service_test.go b/common/service_test.go index 6e24dad6a..ef360a648 100644 --- a/common/service_test.go +++ b/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) } From 57fea1335a7bf898349bcbc9861a05b91625bf35 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Wed, 29 Nov 2017 03:05:20 +0000 Subject: [PATCH 4/5] Makefile and linter --- Makefile | 17 +++++++++-------- circle.yml | 2 +- test.sh | 7 +++++-- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/Makefile b/Makefile index 25773ed36..dd4711aac 100644 --- a/Makefile +++ b/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 \ diff --git a/circle.yml b/circle.yml index 3dba976be..104cfa6f3 100644 --- a/circle.yml +++ b/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}" diff --git a/test.sh b/test.sh index 012162b07..02bdaae86 100755 --- a/test.sh +++ b/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 From 4d991acae0f0eb0ebfab14eabb55e18854c5a2a2 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Wed, 29 Nov 2017 05:16:15 +0000 Subject: [PATCH 5/5] common: comments for Service --- common/service.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/common/service.go b/common/service.go index 608f8b722..d70d16a80 100644 --- a/common/service.go +++ b/common/service.go @@ -13,18 +13,29 @@ var ( ErrAlreadyStopped = errors.New("already stopped") ) +// Service defines a service that can be started, stopped, and reset. type Service interface { + // 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 the service. + // If it's already stopped, will return an error. + // OnStop must never error. Stop() error OnStop() + // 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)