From 4123d54bf69cdd764bbc2b53ae545b4367267645 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 6 Nov 2017 12:18:04 -0500 Subject: [PATCH 01/61] 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 8d4de30a..3973adab 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 dee50e5b..87db2a30 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 02/61] 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 3973adab..32f531d1 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 4ea6340f1ac343d2dafb606cf11ee1c971c5a8ef Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Sat, 11 Nov 2017 11:25:30 -0500 Subject: [PATCH 03/61] add .editorconfig --- .editorconfig | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 .editorconfig diff --git a/.editorconfig b/.editorconfig new file mode 100644 index 00000000..82f77436 --- /dev/null +++ b/.editorconfig @@ -0,0 +1,19 @@ +# top-most EditorConfig file +root = true + +# Unix-style newlines with a newline ending every file +[*] +charset = utf-8 +end_of_line = lf +insert_final_newline = true +trim_trailing_whitespace = true + +[Makefile] +indent_style = tab + +[*.sh] +indent_style = tab + +[*.proto] +indent_style = space +indent_size = 2 From 135a1a7cd78215105a55308c167b3331c225e00b Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Mon, 20 Nov 2017 03:06:18 +0000 Subject: [PATCH 04/61] db: sort keys for memdb iterator --- db/mem_db.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/db/mem_db.go b/db/mem_db.go index 07742750..2f507321 100644 --- a/db/mem_db.go +++ b/db/mem_db.go @@ -2,6 +2,7 @@ package db import ( "fmt" + "sort" "strings" "sync" ) @@ -127,6 +128,8 @@ func (db *MemDB) IteratorPrefix(prefix []byte) Iterator { it.keys = append(it.keys, key) } } + // and we need to sort them + sort.Strings(it.keys) return it } From d3bac7a6fefaeaec662c8b8483c1728ba2bd746c Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Mon, 27 Nov 2017 19:49:30 +0000 Subject: [PATCH 05/61] clist: reduce numTimes in test --- clist/clist_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clist/clist_test.go b/clist/clist_test.go index 2063cf46..9d5272de 100644 --- a/clist/clist_test.go +++ b/clist/clist_test.go @@ -149,7 +149,7 @@ func _TestGCRandom(t *testing.T) { func TestScanRightDeleteRandom(t *testing.T) { const numElements = 10000 - const numTimes = 100000 + const numTimes = 1000 const numScanners = 10 l := New() From 4e705a3157512d757e459c2c86dd2d38068e7bc0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 27 Nov 2017 21:37:15 -0600 Subject: [PATCH 06/61] update changelog --- CHANGELOG.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c380fdcd..b0aa90d7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,17 @@ # Changelog +## 0.4.1 (November 27, 2017) + +FEATURES: + - [common] `Keys()` method on `CMap` + +IMPROVEMENTS: + - [log] complex types now encoded as "%+v" by default if `String()` method is undefined (previously resulted in error) + - [log] logger logs its own errors + +BUG FIXES: + - [common] fixed `Kill()` to build on Windows (Windows does not have `syscall.Kill`) + ## 0.4.0 (October 26, 2017) BREAKING: From 3244f73f32497457987770d4b76523f9d3afdfe9 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 27 Nov 2017 21:37:39 -0600 Subject: [PATCH 07/61] update version --- version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/version.go b/version/version.go index c1635d20..c30887b4 100644 --- a/version/version.go +++ b/version/version.go @@ -1,3 +1,3 @@ package version -const Version = "0.4.0" +const Version = "0.4.1" From e07ad01f62e01016cdcf1e3e05ffb1a6cc8c5b8f Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Mon, 27 Nov 2017 21:24:42 -0700 Subject: [PATCH 08/61] remove package process Fixes https://github.com/tendermint/tmlibs/issues/81 That package is untested and racy, and not used except in a test, but even that's now gutted with https://github.com/tendermint/abci/pull/139 so the general consensus is that we sunset this package. --- process/process.go | 76 ---------------------------------------------- process/util.go | 22 -------------- 2 files changed, 98 deletions(-) delete mode 100644 process/process.go delete mode 100644 process/util.go diff --git a/process/process.go b/process/process.go deleted file mode 100644 index 7d2ae914..00000000 --- a/process/process.go +++ /dev/null @@ -1,76 +0,0 @@ -package process - -import ( - "fmt" - "io" - "os" - "os/exec" - "time" -) - -type Process struct { - Label string - ExecPath string - Args []string - Pid int - StartTime time.Time - EndTime time.Time - Cmd *exec.Cmd `json:"-"` - ExitState *os.ProcessState `json:"-"` - InputFile io.Reader `json:"-"` - OutputFile io.WriteCloser `json:"-"` - WaitCh chan struct{} `json:"-"` -} - -// execPath: command name -// args: args to command. (should not include name) -func StartProcess(label string, dir string, execPath string, args []string, inFile io.Reader, outFile io.WriteCloser) (*Process, error) { - cmd := exec.Command(execPath, args...) - cmd.Dir = dir - cmd.Stdout = outFile - cmd.Stderr = outFile - cmd.Stdin = inFile - if err := cmd.Start(); err != nil { - return nil, err - } - proc := &Process{ - Label: label, - ExecPath: execPath, - Args: args, - Pid: cmd.Process.Pid, - StartTime: time.Now(), - Cmd: cmd, - ExitState: nil, - InputFile: inFile, - OutputFile: outFile, - WaitCh: make(chan struct{}), - } - go func() { - err := proc.Cmd.Wait() - if err != nil { - // fmt.Printf("Process exit: %v\n", err) - if exitError, ok := err.(*exec.ExitError); ok { - proc.ExitState = exitError.ProcessState - } - } - proc.ExitState = proc.Cmd.ProcessState - proc.EndTime = time.Now() // TODO make this goroutine-safe - err = proc.OutputFile.Close() - if err != nil { - fmt.Printf("Error closing output file for %v: %v\n", proc.Label, err) - } - close(proc.WaitCh) - }() - return proc, nil -} - -func (proc *Process) StopProcess(kill bool) error { - defer proc.OutputFile.Close() - if kill { - // fmt.Printf("Killing process %v\n", proc.Cmd.Process) - return proc.Cmd.Process.Kill() - } else { - // fmt.Printf("Stopping process %v\n", proc.Cmd.Process) - return proc.Cmd.Process.Signal(os.Interrupt) - } -} diff --git a/process/util.go b/process/util.go deleted file mode 100644 index 24cf3528..00000000 --- a/process/util.go +++ /dev/null @@ -1,22 +0,0 @@ -package process - -import ( - . "github.com/tendermint/tmlibs/common" -) - -// Runs a command and gets the result. -func Run(dir string, command string, args []string) (string, bool, error) { - outFile := NewBufferCloser(nil) - proc, err := StartProcess("", dir, command, args, nil, outFile) - if err != nil { - return "", false, err - } - - <-proc.WaitCh - - if proc.ExitState.Success() { - return outFile.String(), true, nil - } else { - return outFile.String(), false, nil - } -} From c2fcc093b28e8c3c9ba99d0617127060c3c2e917 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 27 Nov 2017 23:42:36 -0600 Subject: [PATCH 09/61] remove bool from Service#Reset --- common/service.go | 9 ++++---- common/service_test.go | 48 ++++++++++++++++++++++++++++++++++-------- 2 files changed, 44 insertions(+), 13 deletions(-) diff --git a/common/service.go b/common/service.go index 32f531d1..608f8b72 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 6e24dad6..ef360a64 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 10/61] 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 25773ed3..dd4711aa 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 3dba976b..104cfa6f 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 012162b0..02bdaae8 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 11/61] common: comments for Service --- common/service.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/common/service.go b/common/service.go index 608f8b72..d70d16a8 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) From 33abe87c5bcf9e3e41ca4030cdce63e7250c6870 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 29 Nov 2017 12:18:03 -0600 Subject: [PATCH 12/61] IntInSlice and StringInSlice functions Refs https://github.com/tendermint/tendermint/pull/835 --- common/int.go | 10 ++++++++++ common/int_test.go | 14 ++++++++++++++ common/string.go | 10 ++++++++++ common/string_test.go | 14 ++++++++++++++ 4 files changed, 48 insertions(+) create mode 100644 common/int_test.go create mode 100644 common/string_test.go diff --git a/common/int.go b/common/int.go index 756e38cd..a8a5f1e0 100644 --- a/common/int.go +++ b/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 +} diff --git a/common/int_test.go b/common/int_test.go new file mode 100644 index 00000000..1ecc7844 --- /dev/null +++ b/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{})) +} diff --git a/common/string.go b/common/string.go index 1ab91f15..6924e6a5 100644 --- a/common/string.go +++ b/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 +} diff --git a/common/string_test.go b/common/string_test.go new file mode 100644 index 00000000..a82f1022 --- /dev/null +++ b/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{})) +} From 382272798165ac5d24bbedf112744b83e218838c Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 27 Nov 2017 18:40:51 -0600 Subject: [PATCH 13/61] add Conditions function Refs https://github.com/tendermint/tendermint/pull/835 --- pubsub/query/query.go | 146 +++++++++++++++++++++++++++++-------- pubsub/query/query_test.go | 21 ++++++ 2 files changed, 135 insertions(+), 32 deletions(-) diff --git a/pubsub/query/query.go b/pubsub/query/query.go index fdfb87d7..56f2829d 100644 --- a/pubsub/query/query.go +++ b/pubsub/query/query.go @@ -22,6 +22,14 @@ type Query struct { parser *QueryParser } +// Condition represents a single condition within a query and consists of tag +// (e.g. "tx.gas"), operator (e.g. "=") and operand (e.g. "7"). +type Condition struct { + Tag string + Op Operator + Operand interface{} +} + // New parses the given string and returns a query or error if the string is // invalid. func New(s string) (*Query, error) { @@ -48,17 +56,91 @@ func (q *Query) String() string { return q.str } -type operator uint8 +// Operator is an operator that defines some kind of relation between tag and +// operand (equality, etc.). +type Operator uint8 const ( - opLessEqual operator = iota - opGreaterEqual - opLess - opGreater - opEqual - opContains + // "<=" + OpLessEqual Operator = iota + // ">=" + OpGreaterEqual + // "<" + OpLess + // ">" + OpGreater + // "=" + OpEqual + // "CONTAINS"; used to check if a string contains a certain sub string. + OpContains ) +// Conditions returns a list of conditions. +func (q *Query) Conditions() []Condition { + conditions := make([]Condition, 0) + + buffer, begin, end := q.parser.Buffer, 0, 0 + + var tag string + var op Operator + + // tokens must be in the following order: tag ("tx.gas") -> operator ("=") -> operand ("7") + for _, token := range q.parser.Tokens() { + switch token.pegRule { + + case rulePegText: + begin, end = int(token.begin), int(token.end) + case ruletag: + tag = buffer[begin:end] + case rulele: + op = OpLessEqual + case rulege: + op = OpGreaterEqual + case rulel: + op = OpLess + case ruleg: + op = OpGreater + case ruleequal: + op = OpEqual + case rulecontains: + op = OpContains + case rulevalue: + // strip single quotes from value (i.e. "'NewBlock'" -> "NewBlock") + valueWithoutSingleQuotes := buffer[begin+1 : end-1] + conditions = append(conditions, Condition{tag, op, valueWithoutSingleQuotes}) + case rulenumber: + number := buffer[begin:end] + if strings.Contains(number, ".") { // if it looks like a floating-point number + value, err := strconv.ParseFloat(number, 64) + if err != nil { + panic(fmt.Sprintf("got %v while trying to parse %s as float64 (should never happen if the grammar is correct)", err, number)) + } + conditions = append(conditions, Condition{tag, op, value}) + } else { + value, err := strconv.ParseInt(number, 10, 64) + if err != nil { + panic(fmt.Sprintf("got %v while trying to parse %s as int64 (should never happen if the grammar is correct)", err, number)) + } + conditions = append(conditions, Condition{tag, op, value}) + } + case ruletime: + value, err := time.Parse(time.RFC3339, buffer[begin:end]) + if err != nil { + panic(fmt.Sprintf("got %v while trying to parse %s as time.Time / RFC3339 (should never happen if the grammar is correct)", err, buffer[begin:end])) + } + conditions = append(conditions, Condition{tag, op, value}) + case ruledate: + value, err := time.Parse("2006-01-02", buffer[begin:end]) + if err != nil { + panic(fmt.Sprintf("got %v while trying to parse %s as time.Time / '2006-01-02' (should never happen if the grammar is correct)", err, buffer[begin:end])) + } + conditions = append(conditions, Condition{tag, op, value}) + } + } + + return conditions +} + // Matches returns true if the query matches the given set of tags, false otherwise. // // For example, query "name=John" matches tags = {"name": "John"}. More @@ -71,7 +153,7 @@ func (q *Query) Matches(tags map[string]interface{}) bool { buffer, begin, end := q.parser.Buffer, 0, 0 var tag string - var op operator + var op Operator // tokens must be in the following order: tag ("tx.gas") -> operator ("=") -> operand ("7") for _, token := range q.parser.Tokens() { @@ -82,17 +164,17 @@ func (q *Query) Matches(tags map[string]interface{}) bool { case ruletag: tag = buffer[begin:end] case rulele: - op = opLessEqual + op = OpLessEqual case rulege: - op = opGreaterEqual + op = OpGreaterEqual case rulel: - op = opLess + op = OpLess case ruleg: - op = opGreater + op = OpGreater case ruleequal: - op = opEqual + op = OpEqual case rulecontains: - op = opContains + op = OpContains case rulevalue: // strip single quotes from value (i.e. "'NewBlock'" -> "NewBlock") valueWithoutSingleQuotes := buffer[begin+1 : end-1] @@ -149,7 +231,7 @@ func (q *Query) Matches(tags map[string]interface{}) bool { // value from it to the operand using the operator. // // "tx.gas", "=", "7", { "tx.gas": 7, "tx.ID": "4AE393495334" } -func match(tag string, op operator, operand reflect.Value, tags map[string]interface{}) bool { +func match(tag string, op Operator, operand reflect.Value, tags map[string]interface{}) bool { // look up the tag from the query in tags value, ok := tags[tag] if !ok { @@ -163,15 +245,15 @@ func match(tag string, op operator, operand reflect.Value, tags map[string]inter return false } switch op { - case opLessEqual: + case OpLessEqual: return v.Before(operandAsTime) || v.Equal(operandAsTime) - case opGreaterEqual: + case OpGreaterEqual: return v.Equal(operandAsTime) || v.After(operandAsTime) - case opLess: + case OpLess: return v.Before(operandAsTime) - case opGreater: + case OpGreater: return v.After(operandAsTime) - case opEqual: + case OpEqual: return v.Equal(operandAsTime) } case reflect.Float64: @@ -197,15 +279,15 @@ func match(tag string, op operator, operand reflect.Value, tags map[string]inter panic(fmt.Sprintf("Incomparable types: %T (%v) vs float64 (%v)", value, value, operandFloat64)) } switch op { - case opLessEqual: + case OpLessEqual: return v <= operandFloat64 - case opGreaterEqual: + case OpGreaterEqual: return v >= operandFloat64 - case opLess: + case OpLess: return v < operandFloat64 - case opGreater: + case OpGreater: return v > operandFloat64 - case opEqual: + case OpEqual: return v == operandFloat64 } case reflect.Int64: @@ -231,15 +313,15 @@ func match(tag string, op operator, operand reflect.Value, tags map[string]inter panic(fmt.Sprintf("Incomparable types: %T (%v) vs int64 (%v)", value, value, operandInt)) } switch op { - case opLessEqual: + case OpLessEqual: return v <= operandInt - case opGreaterEqual: + case OpGreaterEqual: return v >= operandInt - case opLess: + case OpLess: return v < operandInt - case opGreater: + case OpGreater: return v > operandInt - case opEqual: + case OpEqual: return v == operandInt } case reflect.String: @@ -248,9 +330,9 @@ func match(tag string, op operator, operand reflect.Value, tags map[string]inter return false } switch op { - case opEqual: + case OpEqual: return v == operand.String() - case opContains: + case OpContains: return strings.Contains(v, operand.String()) } default: diff --git a/pubsub/query/query_test.go b/pubsub/query/query_test.go index 431ae1fe..93b63a15 100644 --- a/pubsub/query/query_test.go +++ b/pubsub/query/query_test.go @@ -62,3 +62,24 @@ func TestMustParse(t *testing.T) { assert.Panics(t, func() { query.MustParse("=") }) assert.NotPanics(t, func() { query.MustParse("tm.events.type='NewBlock'") }) } + +func TestConditions(t *testing.T) { + txTime, err := time.Parse(time.RFC3339, "2013-05-03T14:45:00Z") + require.NoError(t, err) + + testCases := []struct { + s string + conditions []query.Condition + }{ + {"tm.events.type='NewBlock'", []query.Condition{query.Condition{"tm.events.type", query.OpEqual, "NewBlock"}}}, + {"tx.gas > 7 AND tx.gas < 9", []query.Condition{query.Condition{"tx.gas", query.OpGreater, int64(7)}, query.Condition{"tx.gas", query.OpLess, int64(9)}}}, + {"tx.time >= TIME 2013-05-03T14:45:00Z", []query.Condition{query.Condition{"tx.time", query.OpGreaterEqual, txTime}}}, + } + + for _, tc := range testCases { + query, err := query.New(tc.s) + require.Nil(t, err) + + assert.Equal(t, tc.conditions, query.Conditions()) + } +} From c9694b1ba1452403a521c3952537528ce64c9a96 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 27 Nov 2017 18:46:57 -0600 Subject: [PATCH 14/61] fix warnings --- pubsub/query/query_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pubsub/query/query_test.go b/pubsub/query/query_test.go index 93b63a15..b16abdaf 100644 --- a/pubsub/query/query_test.go +++ b/pubsub/query/query_test.go @@ -45,15 +45,15 @@ func TestMatches(t *testing.T) { } for _, tc := range testCases { - query, err := query.New(tc.s) + q, err := query.New(tc.s) if !tc.err { require.Nil(t, err) } if tc.matches { - assert.True(t, query.Matches(tc.tags), "Query '%s' should match %v", tc.s, tc.tags) + assert.True(t, q.Matches(tc.tags), "Query '%s' should match %v", tc.s, tc.tags) } else { - assert.False(t, query.Matches(tc.tags), "Query '%s' should not match %v", tc.s, tc.tags) + assert.False(t, q.Matches(tc.tags), "Query '%s' should not match %v", tc.s, tc.tags) } } } @@ -77,9 +77,9 @@ func TestConditions(t *testing.T) { } for _, tc := range testCases { - query, err := query.New(tc.s) + q, err := query.New(tc.s) require.Nil(t, err) - assert.Equal(t, tc.conditions, query.Conditions()) + assert.Equal(t, tc.conditions, q.Conditions()) } } From ebc543ebe3598045b15506984d99acc138ec06eb Mon Sep 17 00:00:00 2001 From: Petabyte Storage Date: Fri, 1 Dec 2017 09:51:37 -0800 Subject: [PATCH 15/61] fix warnings --- pubsub/query/parser_test.go | 4 ++-- pubsub/query/query_test.go | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/pubsub/query/parser_test.go b/pubsub/query/parser_test.go index 165ddda7..e31079b4 100644 --- a/pubsub/query/parser_test.go +++ b/pubsub/query/parser_test.go @@ -83,9 +83,9 @@ func TestParser(t *testing.T) { for _, c := range cases { _, err := query.New(c.query) if c.valid { - assert.NoError(t, err, "Query was '%s'", c.query) + assert.NoErrorf(t, err, "Query was '%s'", c.query) } else { - assert.Error(t, err, "Query was '%s'", c.query) + assert.Errorf(t, err, "Query was '%s'", c.query) } } } diff --git a/pubsub/query/query_test.go b/pubsub/query/query_test.go index b16abdaf..b980a79c 100644 --- a/pubsub/query/query_test.go +++ b/pubsub/query/query_test.go @@ -71,9 +71,9 @@ func TestConditions(t *testing.T) { s string conditions []query.Condition }{ - {"tm.events.type='NewBlock'", []query.Condition{query.Condition{"tm.events.type", query.OpEqual, "NewBlock"}}}, - {"tx.gas > 7 AND tx.gas < 9", []query.Condition{query.Condition{"tx.gas", query.OpGreater, int64(7)}, query.Condition{"tx.gas", query.OpLess, int64(9)}}}, - {"tx.time >= TIME 2013-05-03T14:45:00Z", []query.Condition{query.Condition{"tx.time", query.OpGreaterEqual, txTime}}}, + {s: "tm.events.type='NewBlock'", conditions: []query.Condition{query.Condition{Tag: "tm.events.type", Op: query.OpEqual, Operand: "NewBlock"}}}, + {s: "tx.gas > 7 AND tx.gas < 9", conditions: []query.Condition{query.Condition{Tag: "tx.gas", Op: query.OpGreater, Operand: int64(7)}, query.Condition{Tag: "tx.gas", Op: query.OpLess, Operand: int64(9)}}}, + {s: "tx.time >= TIME 2013-05-03T14:45:00Z", conditions: []query.Condition{query.Condition{Tag: "tx.time", Op: query.OpGreaterEqual, Operand: txTime}}}, } for _, tc := range testCases { From 3af6044fdf1945a37d81af4f7703e4a2c24ebfbc Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Mon, 4 Dec 2017 10:38:18 -0600 Subject: [PATCH 16/61] add license file (Fixes #87) [ci skip] --- LICENSE | 193 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 193 insertions(+) create mode 100644 LICENSE diff --git a/LICENSE b/LICENSE new file mode 100644 index 00000000..5d4ad3b1 --- /dev/null +++ b/LICENSE @@ -0,0 +1,193 @@ +Tendermint Libraries +Copyright (C) 2015 Tendermint + + + + Apache License + Version 2.0, January 2004 + https://www.apache.org/licenses/ + + TERMS AND CONDITIONS FOR USE, REPRODUCTION, AND DISTRIBUTION + + 1. Definitions. + + "License" shall mean the terms and conditions for use, reproduction, + and distribution as defined by Sections 1 through 9 of this document. + + "Licensor" shall mean the copyright owner or entity authorized by + the copyright owner that is granting the License. + + "Legal Entity" shall mean the union of the acting entity and all + other entities that control, are controlled by, or are under common + control with that entity. For the purposes of this definition, + "control" means (i) the power, direct or indirect, to cause the + direction or management of such entity, whether by contract or + otherwise, or (ii) ownership of fifty percent (50%) or more of the + outstanding shares, or (iii) beneficial ownership of such entity. + + "You" (or "Your") shall mean an individual or Legal Entity + exercising permissions granted by this License. + + "Source" form shall mean the preferred form for making modifications, + including but not limited to software source code, documentation + source, and configuration files. + + "Object" form shall mean any form resulting from mechanical + transformation or translation of a Source form, including but + not limited to compiled object code, generated documentation, + and conversions to other media types. + + "Work" shall mean the work of authorship, whether in Source or + Object form, made available under the License, as indicated by a + copyright notice that is included in or attached to the work + (an example is provided in the Appendix below). + + "Derivative Works" shall mean any work, whether in Source or Object + form, that is based on (or derived from) the Work and for which the + editorial revisions, annotations, elaborations, or other modifications + represent, as a whole, an original work of authorship. For the purposes + of this License, Derivative Works shall not include works that remain + separable from, or merely link (or bind by name) to the interfaces of, + the Work and Derivative Works thereof. + + "Contribution" shall mean any work of authorship, including + the original version of the Work and any modifications or additions + to that Work or Derivative Works thereof, that is intentionally + submitted to Licensor for inclusion in the Work by the copyright owner + or by an individual or Legal Entity authorized to submit on behalf of + the copyright owner. For the purposes of this definition, "submitted" + means any form of electronic, verbal, or written communication sent + to the Licensor or its representatives, including but not limited to + communication on electronic mailing lists, source code control systems, + and issue tracking systems that are managed by, or on behalf of, the + Licensor for the purpose of discussing and improving the Work, but + excluding communication that is conspicuously marked or otherwise + designated in writing by the copyright owner as "Not a Contribution." + + "Contributor" shall mean Licensor and any individual or Legal Entity + on behalf of whom a Contribution has been received by Licensor and + subsequently incorporated within the Work. + + 2. Grant of Copyright License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + copyright license to reproduce, prepare Derivative Works of, + publicly display, publicly perform, sublicense, and distribute the + Work and such Derivative Works in Source or Object form. + + 3. Grant of Patent License. Subject to the terms and conditions of + this License, each Contributor hereby grants to You a perpetual, + worldwide, non-exclusive, no-charge, royalty-free, irrevocable + (except as stated in this section) patent license to make, have made, + use, offer to sell, sell, import, and otherwise transfer the Work, + where such license applies only to those patent claims licensable + by such Contributor that are necessarily infringed by their + Contribution(s) alone or by combination of their Contribution(s) + with the Work to which such Contribution(s) was submitted. If You + institute patent litigation against any entity (including a + cross-claim or counterclaim in a lawsuit) alleging that the Work + or a Contribution incorporated within the Work constitutes direct + or contributory patent infringement, then any patent licenses + granted to You under this License for that Work shall terminate + as of the date such litigation is filed. + + 4. Redistribution. You may reproduce and distribute copies of the + Work or Derivative Works thereof in any medium, with or without + modifications, and in Source or Object form, provided that You + meet the following conditions: + + (a) You must give any other recipients of the Work or + Derivative Works a copy of this License; and + + (b) You must cause any modified files to carry prominent notices + stating that You changed the files; and + + (c) You must retain, in the Source form of any Derivative Works + that You distribute, all copyright, patent, trademark, and + attribution notices from the Source form of the Work, + excluding those notices that do not pertain to any part of + the Derivative Works; and + + (d) If the Work includes a "NOTICE" text file as part of its + distribution, then any Derivative Works that You distribute must + include a readable copy of the attribution notices contained + within such NOTICE file, excluding those notices that do not + pertain to any part of the Derivative Works, in at least one + of the following places: within a NOTICE text file distributed + as part of the Derivative Works; within the Source form or + documentation, if provided along with the Derivative Works; or, + within a display generated by the Derivative Works, if and + wherever such third-party notices normally appear. The contents + of the NOTICE file are for informational purposes only and + do not modify the License. You may add Your own attribution + notices within Derivative Works that You distribute, alongside + or as an addendum to the NOTICE text from the Work, provided + that such additional attribution notices cannot be construed + as modifying the License. + + You may add Your own copyright statement to Your modifications and + may provide additional or different license terms and conditions + for use, reproduction, or distribution of Your modifications, or + for any such Derivative Works as a whole, provided Your use, + reproduction, and distribution of the Work otherwise complies with + the conditions stated in this License. + + 5. Submission of Contributions. Unless You explicitly state otherwise, + any Contribution intentionally submitted for inclusion in the Work + by You to the Licensor shall be under the terms and conditions of + this License, without any additional terms or conditions. + Notwithstanding the above, nothing herein shall supersede or modify + the terms of any separate license agreement you may have executed + with Licensor regarding such Contributions. + + 6. Trademarks. This License does not grant permission to use the trade + names, trademarks, service marks, or product names of the Licensor, + except as required for reasonable and customary use in describing the + origin of the Work and reproducing the content of the NOTICE file. + + 7. Disclaimer of Warranty. Unless required by applicable law or + agreed to in writing, Licensor provides the Work (and each + Contributor provides its Contributions) on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or + implied, including, without limitation, any warranties or conditions + of TITLE, NON-INFRINGEMENT, MERCHANTABILITY, or FITNESS FOR A + PARTICULAR PURPOSE. You are solely responsible for determining the + appropriateness of using or redistributing the Work and assume any + risks associated with Your exercise of permissions under this License. + + 8. Limitation of Liability. In no event and under no legal theory, + whether in tort (including negligence), contract, or otherwise, + unless required by applicable law (such as deliberate and grossly + negligent acts) or agreed to in writing, shall any Contributor be + liable to You for damages, including any direct, indirect, special, + incidental, or consequential damages of any character arising as a + result of this License or out of the use or inability to use the + Work (including but not limited to damages for loss of goodwill, + work stoppage, computer failure or malfunction, or any and all + other commercial damages or losses), even if such Contributor + has been advised of the possibility of such damages. + + 9. Accepting Warranty or Additional Liability. While redistributing + the Work or Derivative Works thereof, You may choose to offer, + and charge a fee for, acceptance of support, warranty, indemnity, + or other liability obligations and/or rights consistent with this + License. However, in accepting such obligations, You may act only + on Your own behalf and on Your sole responsibility, not on behalf + of any other Contributor, and only if You agree to indemnify, + defend, and hold each Contributor harmless for any liability + incurred by, or claims asserted against, such Contributor by reason + of your accepting any such warranty or additional liability. + + END OF TERMS AND CONDITIONS + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + https://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. From 4769719a4b3fbe401b9e5bdf49418d32fa027119 Mon Sep 17 00:00:00 2001 From: Petabyte Storage Date: Mon, 4 Dec 2017 08:54:19 -0800 Subject: [PATCH 17/61] fix Errorf --- glide.lock | 40 ++++++++++++++++++------------------- pubsub/query/parser_test.go | 4 ++-- 2 files changed, 21 insertions(+), 23 deletions(-) diff --git a/glide.lock b/glide.lock index b0b3ff3c..9f8ddf0e 100644 --- a/glide.lock +++ b/glide.lock @@ -1,10 +1,10 @@ hash: 6efda1f3891a7211fc3dc1499c0079267868ced9739b781928af8e225420f867 -updated: 2017-08-11T20:28:34.550901198Z +updated: 2017-12-04T08:45:29.247829134-08:00 imports: - name: github.com/fsnotify/fsnotify version: 4da3e2cfbabc9f751898f250b49f2439785783a1 - name: github.com/go-kit/kit - version: 0873e56b0faeae3a1d661b10d629135508ea5504 + version: d67bb4c202e3b91377d1079b110a6c9ce23ab2f8 subpackages: - log - log/level @@ -18,11 +18,11 @@ imports: - name: github.com/go-playground/universal-translator version: 71201497bace774495daed26a3874fd339e0b538 - name: github.com/go-stack/stack - version: 7a2f19628aabfe68f0766b59e74d6315f8347d22 + version: 100eb0c0a9c5b306ca2fb4f165df21d80ada4b82 - name: github.com/golang/snappy version: 553a641470496b2327abcac10b36396bd98e45c9 - name: github.com/hashicorp/hcl - version: a4b07c25de5ff55ad3b8936cea69a79a3d95a855 + version: 23c074d0eceb2b8a5bfdbb271ab780cde70f05a8 subpackages: - hcl/ast - hcl/parser @@ -39,33 +39,31 @@ imports: - name: github.com/kr/logfmt version: b84e30acd515aadc4b783ad4ff83aff3299bdfe0 - name: github.com/magiconair/properties - version: 51463bfca2576e06c62a8504b5c0f06d61312647 + version: 8d7837e64d3c1ee4e54a880c5a920ab4316fc90a - name: github.com/mattn/go-colorable - version: ded68f7a9561c023e790de24279db7ebf473ea80 + version: 6fcc0c1fd9b620311d821b106a400b35dc95c497 - name: github.com/mattn/go-isatty - version: fc9e8d8ef48496124e79ae0df75490096eccf6fe + version: a5cdd64afdee435007ee3e9f6ed4684af949d568 - name: github.com/mitchellh/mapstructure - version: cc8532a8e9a55ea36402aa21efdf403a60d34096 -- name: github.com/pelletier/go-buffruneio - version: c37440a7cf42ac63b919c752ca73a85067e05992 + version: 06020f85339e21b2478f756a78e295255ffa4d6a - name: github.com/pelletier/go-toml - version: 97253b98df84f9eef872866d079e74b8265150f1 + version: 4e9e0ee19b60b13eb79915933f44d8ed5f268bdd - name: github.com/pkg/errors - version: c605e284fe17294bda444b34710735b29d1a9d90 + version: 645ef00459ed84a119197bfb8d8205042c6df63d - name: github.com/spf13/afero - version: 9be650865eab0c12963d8753212f4f9c66cdcf12 + version: 5660eeed305fe5f69c8fc6cf899132a459a97064 subpackages: - mem - name: github.com/spf13/cast version: acbeb36b902d72a7a4c18e8f3241075e7ab763e4 - name: github.com/spf13/cobra - version: db6b9a8b3f3f400c8ecb4a4d7d02245b8facad66 + version: 7b2c5ac9fc04fc5efafb60700713d4fa609b777b - name: github.com/spf13/jwalterweatherman - version: fa7ca7e836cf3a8bb4ebf799f472c12d7e903d66 + version: 12bd96e66386c1960ab0f74ced1362f66f552f7b - name: github.com/spf13/pflag version: 80fe0fb4eba54167e2ccae1c6c950e72abf61b73 - name: github.com/spf13/viper - version: 0967fc9aceab2ce9da34061253ac10fb99bba5b2 + version: 25b30aa063fc18e48662b86996252eabdcf2f0c7 - name: github.com/syndtr/goleveldb version: 8c81ea47d4c41a385645e133e15510fc6a2a74b4 subpackages: @@ -82,7 +80,7 @@ imports: - leveldb/table - leveldb/util - name: github.com/tendermint/go-wire - version: b53add0b622662731985485f3a19be7f684660b8 + version: 7d50b38b3815efe313728de77e2995c8813ce13f subpackages: - data - data/base58 @@ -91,11 +89,11 @@ imports: subpackages: - term - name: golang.org/x/crypto - version: 5a033cc77e57eca05bdb50522851d29e03569cbe + version: c7af5bf2638a1164f2eb5467c39c6cffbd13a02e subpackages: - ripemd160 - name: golang.org/x/sys - version: 9ccfe848b9db8435a24c424abbc07a921adf1df5 + version: 661970f62f5897bc0cd5fdca7e087ba8a98a8fa1 subpackages: - unix - name: golang.org/x/text @@ -104,9 +102,9 @@ imports: - transform - unicode/norm - name: gopkg.in/go-playground/validator.v9 - version: d529ee1b0f30352444f507cc6cdac96bfd12decc + version: 6d8c18553ea1ac493d049edd6f102f52e618f085 - name: gopkg.in/yaml.v2 - version: cd8b52f8269e0feb286dfeef29f8fe4d5b397e0b + version: eb3733d160e74a9c7e442f435eb3bea458e1d19f testImports: - name: github.com/davecgh/go-spew version: 6d212800a42e8ab5c146b8ace3490ee17e5225f9 diff --git a/pubsub/query/parser_test.go b/pubsub/query/parser_test.go index e31079b4..bf562c0b 100644 --- a/pubsub/query/parser_test.go +++ b/pubsub/query/parser_test.go @@ -83,9 +83,9 @@ func TestParser(t *testing.T) { for _, c := range cases { _, err := query.New(c.query) if c.valid { - assert.NoErrorf(t, err, "Query was '%s'", c.query) + assert.NoError(t, err, "Query was '"+c.query+"'") } else { - assert.Errorf(t, err, "Query was '%s'", c.query) + assert.Error(t, err, "Query was '"+c.query+"'") } } } From 53cdb6cf82148a7a0c52d959e152d72797f3df87 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 5 Dec 2017 11:25:17 +0100 Subject: [PATCH 18/61] Demo throttle timer is broken --- common/throttle_timer_test.go | 78 +++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 common/throttle_timer_test.go diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go new file mode 100644 index 00000000..bd03a7a6 --- /dev/null +++ b/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 counter struct { + input chan struct{} + mtx sync.Mutex + count int +} + +func (c *counter) Increment() { + c.mtx.Lock() + c.count++ + c.mtx.Unlock() +} + +func (c *counter) 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 *counter) 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 := &counter{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) +} From 26abd65e34910994e921020335ed0d5c01a113a9 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Tue, 5 Dec 2017 15:01:07 +0100 Subject: [PATCH 19/61] Add tests for repeat timer --- common/repeat_timer_test.go | 78 +++++++++++++++++++++++++++++++++++ common/throttle_timer_test.go | 10 ++--- 2 files changed, 83 insertions(+), 5 deletions(-) create mode 100644 common/repeat_timer_test.go diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go new file mode 100644 index 00000000..9f03f41d --- /dev/null +++ b/common/repeat_timer_test.go @@ -0,0 +1,78 @@ +package common + +import ( + "sync" + "testing" + "time" + + // make govet noshadow happy... + asrt "github.com/stretchr/testify/assert" +) + +type rCounter struct { + input chan time.Time + mtx sync.Mutex + count int +} + +func (c *rCounter) Increment() { + c.mtx.Lock() + c.count++ + c.mtx.Unlock() +} + +func (c *rCounter) 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 *rCounter) Read() { + for range c.input { + c.Increment() + } +} + +func TestRepeat(test *testing.T) { + assert := asrt.New(test) + + dur := time.Duration(50) * time.Millisecond + short := time.Duration(20) * time.Millisecond + // delay waits for cnt durations, an a little extra + delay := func(cnt int) time.Duration { + return time.Duration(cnt)*dur + time.Millisecond + } + t := NewRepeatTimer("bar", dur) + + // start at 0 + c := &rCounter{input: t.Ch} + go c.Read() + assert.Equal(0, c.Count()) + + // wait for 4 periods + time.Sleep(delay(4)) + assert.Equal(4, c.Count()) + + // keep reseting leads to no firing + for i := 0; i < 20; i++ { + time.Sleep(short) + t.Reset() + } + assert.Equal(4, c.Count()) + + // after this, it still works normal + time.Sleep(delay(2)) + assert.Equal(6, c.Count()) + + // after a stop, nothing more is sent + stopped := t.Stop() + assert.True(stopped) + time.Sleep(delay(7)) + assert.Equal(6, c.Count()) + + // close channel to stop counter + close(t.Ch) +} diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index bd03a7a6..00f5abde 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -9,19 +9,19 @@ import ( asrt "github.com/stretchr/testify/assert" ) -type counter struct { +type thCounter struct { input chan struct{} mtx sync.Mutex count int } -func (c *counter) Increment() { +func (c *thCounter) Increment() { c.mtx.Lock() c.count++ c.mtx.Unlock() } -func (c *counter) Count() int { +func (c *thCounter) Count() int { c.mtx.Lock() val := c.count c.mtx.Unlock() @@ -30,7 +30,7 @@ func (c *counter) Count() int { // Read should run in a go-routine and // updates count by one every time a packet comes in -func (c *counter) Read() { +func (c *thCounter) Read() { for range c.input { c.Increment() } @@ -45,7 +45,7 @@ func TestThrottle(test *testing.T) { t := NewThrottleTimer("foo", delay) // start at 0 - c := &counter{input: t.Ch} + c := &thCounter{input: t.Ch} assert.Equal(0, c.Count()) go c.Read() From c325ce218293417161ac53152419ebc17b48f132 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 5 Dec 2017 17:13:10 -0600 Subject: [PATCH 20/61] use NoErrorf and Errorf functions --- glide.lock | 42 ++++++++++++++++++------------------- glide.yaml | 1 - pubsub/query/parser_test.go | 4 ++-- 3 files changed, 23 insertions(+), 24 deletions(-) diff --git a/glide.lock b/glide.lock index 9f8ddf0e..4b9c46c7 100644 --- a/glide.lock +++ b/glide.lock @@ -1,10 +1,10 @@ -hash: 6efda1f3891a7211fc3dc1499c0079267868ced9739b781928af8e225420f867 -updated: 2017-12-04T08:45:29.247829134-08:00 +hash: 1f3d3426e823e4a8e6d4473615fcc86c767bbea6da9114ea1e7e0a9f0ccfa129 +updated: 2017-12-05T23:47:13.202024407Z imports: - name: github.com/fsnotify/fsnotify version: 4da3e2cfbabc9f751898f250b49f2439785783a1 - name: github.com/go-kit/kit - version: d67bb4c202e3b91377d1079b110a6c9ce23ab2f8 + version: 53f10af5d5c7375d4655a3d6852457ed17ab5cc7 subpackages: - log - log/level @@ -12,13 +12,13 @@ imports: - name: github.com/go-logfmt/logfmt version: 390ab7935ee28ec6b286364bba9b4dd6410cb3d5 - name: github.com/go-playground/locales - version: 1e5f1161c6416a5ff48840eb8724a394e48cc534 + version: e4cbcb5d0652150d40ad0646651076b6bd2be4f6 subpackages: - currency - name: github.com/go-playground/universal-translator version: 71201497bace774495daed26a3874fd339e0b538 - name: github.com/go-stack/stack - version: 100eb0c0a9c5b306ca2fb4f165df21d80ada4b82 + version: 259ab82a6cad3992b4e21ff5cac294ccb06474bc - name: github.com/golang/snappy version: 553a641470496b2327abcac10b36396bd98e45c9 - name: github.com/hashicorp/hcl @@ -39,33 +39,33 @@ imports: - name: github.com/kr/logfmt version: b84e30acd515aadc4b783ad4ff83aff3299bdfe0 - name: github.com/magiconair/properties - version: 8d7837e64d3c1ee4e54a880c5a920ab4316fc90a + version: 49d762b9817ba1c2e9d0c69183c2b4a8b8f1d934 - name: github.com/mattn/go-colorable version: 6fcc0c1fd9b620311d821b106a400b35dc95c497 - name: github.com/mattn/go-isatty - version: a5cdd64afdee435007ee3e9f6ed4684af949d568 + version: 6ca4dbf54d38eea1a992b3c722a76a5d1c4cb25c - name: github.com/mitchellh/mapstructure version: 06020f85339e21b2478f756a78e295255ffa4d6a - name: github.com/pelletier/go-toml version: 4e9e0ee19b60b13eb79915933f44d8ed5f268bdd - name: github.com/pkg/errors - version: 645ef00459ed84a119197bfb8d8205042c6df63d + version: f15c970de5b76fac0b59abb32d62c17cc7bed265 - name: github.com/spf13/afero - version: 5660eeed305fe5f69c8fc6cf899132a459a97064 + version: 8d919cbe7e2627e417f3e45c3c0e489a5b7e2536 subpackages: - mem - name: github.com/spf13/cast version: acbeb36b902d72a7a4c18e8f3241075e7ab763e4 - name: github.com/spf13/cobra - version: 7b2c5ac9fc04fc5efafb60700713d4fa609b777b + version: de2d9c4eca8f3c1de17d48b096b6504e0296f003 - name: github.com/spf13/jwalterweatherman version: 12bd96e66386c1960ab0f74ced1362f66f552f7b - name: github.com/spf13/pflag - version: 80fe0fb4eba54167e2ccae1c6c950e72abf61b73 + version: 4c012f6dcd9546820e378d0bdda4d8fc772cdfea - name: github.com/spf13/viper - version: 25b30aa063fc18e48662b86996252eabdcf2f0c7 + version: 4dddf7c62e16bce5807744018f5b753bfe21bbd2 - name: github.com/syndtr/goleveldb - version: 8c81ea47d4c41a385645e133e15510fc6a2a74b4 + version: adf24ef3f94bd13ec4163060b21a5678f22b429b subpackages: - leveldb - leveldb/cache @@ -80,7 +80,7 @@ imports: - leveldb/table - leveldb/util - name: github.com/tendermint/go-wire - version: 7d50b38b3815efe313728de77e2995c8813ce13f + version: 2baffcb6b690057568bc90ef1d457efb150b979a subpackages: - data - data/base58 @@ -89,25 +89,25 @@ imports: subpackages: - term - name: golang.org/x/crypto - version: c7af5bf2638a1164f2eb5467c39c6cffbd13a02e + version: 94eea52f7b742c7cbe0b03b22f0c4c8631ece122 subpackages: - ripemd160 - name: golang.org/x/sys - version: 661970f62f5897bc0cd5fdca7e087ba8a98a8fa1 + version: 8b4580aae2a0dd0c231a45d3ccb8434ff533b840 subpackages: - unix - name: golang.org/x/text - version: 470f45bf29f4147d6fbd7dfd0a02a848e49f5bf4 + version: 57961680700a5336d15015c8c50686ca5ba362a4 subpackages: - transform - unicode/norm - name: gopkg.in/go-playground/validator.v9 - version: 6d8c18553ea1ac493d049edd6f102f52e618f085 + version: 61caf9d3038e1af346dbf5c2e16f6678e1548364 - name: gopkg.in/yaml.v2 - version: eb3733d160e74a9c7e442f435eb3bea458e1d19f + version: 287cf08546ab5e7e37d55a84f7ed3fd1db036de5 testImports: - name: github.com/davecgh/go-spew - version: 6d212800a42e8ab5c146b8ace3490ee17e5225f9 + version: 04cdfd42973bb9c8589fd6a731800cf222fde1a9 subpackages: - spew - name: github.com/pmezard/go-difflib @@ -115,7 +115,7 @@ testImports: subpackages: - difflib - name: github.com/stretchr/testify - version: 69483b4bd14f5845b5a1e55bca19e954e827f1d0 + version: 2aa2c176b9dab406a6970f6a55f513e8a8c8b18f subpackages: - assert - require diff --git a/glide.yaml b/glide.yaml index 22825a27..0d722c85 100644 --- a/glide.yaml +++ b/glide.yaml @@ -26,7 +26,6 @@ import: - package: gopkg.in/go-playground/validator.v9 testImport: - package: github.com/stretchr/testify - version: ^1.1.4 subpackages: - assert - require diff --git a/pubsub/query/parser_test.go b/pubsub/query/parser_test.go index bf562c0b..e31079b4 100644 --- a/pubsub/query/parser_test.go +++ b/pubsub/query/parser_test.go @@ -83,9 +83,9 @@ func TestParser(t *testing.T) { for _, c := range cases { _, err := query.New(c.query) if c.valid { - assert.NoError(t, err, "Query was '"+c.query+"'") + assert.NoErrorf(t, err, "Query was '%s'", c.query) } else { - assert.Error(t, err, "Query was '"+c.query+"'") + assert.Errorf(t, err, "Query was '%s'", c.query) } } } From 303b6df81274e5d18c9b47088c8318da627d61ea Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 5 Dec 2017 18:04:07 -0600 Subject: [PATCH 21/61] update changelog --- CHANGELOG.md | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0aa90d7..5acfa194 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,16 @@ # Changelog +## 0.5.0 (December 5, 2017) + +BREAKING: + - [common] replace Service#Start, Service#Stop first param (bool) with an + error (ErrAlreadyStarted, ErrAlreadyStopped) + - [common] replace Service#Reset first param (bool) with an error + - [process] removed + +FEATURES: + - [common] IntInSlice and StringInSlice functions + ## 0.4.1 (November 27, 2017) FEATURES: From b166d627f3317e8e547412596ebfb390fd837d85 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 5 Dec 2017 18:04:28 -0600 Subject: [PATCH 22/61] bump up version to 0.5.0 --- version/version.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/version/version.go b/version/version.go index c30887b4..45222da7 100644 --- a/version/version.go +++ b/version/version.go @@ -1,3 +1,3 @@ package version -const Version = "0.4.1" +const Version = "0.5.0" From e6be03db31949be332e13ff460cc4f067575c7f0 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Wed, 6 Dec 2017 02:05:57 -0500 Subject: [PATCH 23/61] update license and changelog --- CHANGELOG.md | 5 +++-- LICENSE | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5acfa194..b679b839 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,13 +3,14 @@ ## 0.5.0 (December 5, 2017) BREAKING: - - [common] replace Service#Start, Service#Stop first param (bool) with an + - [common] replace Service#Start, Service#Stop first return value (bool) with an error (ErrAlreadyStarted, ErrAlreadyStopped) - - [common] replace Service#Reset first param (bool) with an error + - [common] replace Service#Reset first return value (bool) with an error - [process] removed FEATURES: - [common] IntInSlice and StringInSlice functions + - [pubsub/query] introduce `Condition` struct, expose `Operator`, and add `query.Conditions()` ## 0.4.1 (November 27, 2017) diff --git a/LICENSE b/LICENSE index 5d4ad3b1..06bc5e1c 100644 --- a/LICENSE +++ b/LICENSE @@ -1,5 +1,5 @@ Tendermint Libraries -Copyright (C) 2015 Tendermint +Copyright (C) 2017 Tendermint From 3d9113c16e08fe53f31a2403a5280202c8c9cc14 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 09:18:04 +0100 Subject: [PATCH 24/61] Add a bit more padding to tests so they pass on osx with -race --- common/repeat_timer_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 9f03f41d..87f34b95 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -43,7 +43,7 @@ func TestRepeat(test *testing.T) { short := time.Duration(20) * time.Millisecond // delay waits for cnt durations, an a little extra delay := func(cnt int) time.Duration { - return time.Duration(cnt)*dur + time.Millisecond + return time.Duration(cnt)*dur + time.Duration(5)*time.Millisecond } t := NewRepeatTimer("bar", dur) From dcb43956048f0d38495f39e43fd4438ec6d47de7 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 11:17:50 +0100 Subject: [PATCH 25/61] Refactor throttle timer --- common/throttle_timer.go | 104 ++++++++++++++++++++++------------ common/throttle_timer_test.go | 19 ++++++- 2 files changed, 86 insertions(+), 37 deletions(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index 38ef4e9a..e260e01b 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -1,7 +1,7 @@ package common import ( - "sync" + "fmt" "time" ) @@ -12,54 +12,88 @@ If a long continuous burst of .Set() calls happens, ThrottleTimer fires at most once every "dur". */ type ThrottleTimer struct { - Name string - Ch chan struct{} - quit chan struct{} - dur time.Duration + Name string + Ch chan struct{} + input chan command + dur time.Duration - mtx sync.Mutex timer *time.Timer isSet bool } +type command int32 + +const ( + Set command = iota + Unset + Quit +) + func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { - var ch = make(chan struct{}) - var quit = make(chan struct{}) - var t = &ThrottleTimer{Name: name, Ch: ch, dur: dur, quit: quit} - t.mtx.Lock() - t.timer = time.AfterFunc(dur, t.fireRoutine) - t.mtx.Unlock() + var t = &ThrottleTimer{ + Name: name, + Ch: make(chan struct{}, 1), + dur: dur, + input: make(chan command), + timer: time.NewTimer(dur), + } t.timer.Stop() + go t.run() return t } -func (t *ThrottleTimer) fireRoutine() { - t.mtx.Lock() - defer t.mtx.Unlock() - select { - case t.Ch <- struct{}{}: - t.isSet = false - case <-t.quit: - // do nothing - default: - t.timer.Reset(t.dur) +func (t *ThrottleTimer) run() { + for { + select { + case cmd := <-t.input: + // stop goroutine if the input says so + if t.processInput(cmd) { + // TODO: do we want to close the channels??? + // close(t.Ch) + // close(t.input) + return + } + case <-t.timer.C: + t.isSet = false + t.Ch <- struct{}{} + } } } +// all modifications of the internal state of ThrottleTimer +// happen in this method. It is only called from the run goroutine +// so we avoid any race conditions +func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { + fmt.Printf("processInput: %d\n", cmd) + switch cmd { + case Set: + if !t.isSet { + t.isSet = true + t.timer.Reset(t.dur) + } + case Quit: + shutdown = true + fallthrough + case Unset: + if t.isSet { + t.isSet = false + if !t.timer.Stop() { + <-t.timer.C + } + } + default: + panic("unknown command!") + } + // return true + return shutdown +} + func (t *ThrottleTimer) Set() { - t.mtx.Lock() - defer t.mtx.Unlock() - if !t.isSet { - t.isSet = true - t.timer.Reset(t.dur) - } + t.input <- Set } func (t *ThrottleTimer) Unset() { - t.mtx.Lock() - defer t.mtx.Unlock() - t.isSet = false - t.timer.Stop() + t.input <- Unset } // For ease of .Stop()'ing services before .Start()'ing them, @@ -68,8 +102,6 @@ func (t *ThrottleTimer) Stop() bool { if t == nil { return false } - close(t.quit) - t.mtx.Lock() - defer t.mtx.Unlock() - return t.timer.Stop() + t.input <- Quit + return true } diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index 00f5abde..014f9dcd 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -41,6 +41,7 @@ func TestThrottle(test *testing.T) { ms := 50 delay := time.Duration(ms) * time.Millisecond + shortwait := time.Duration(ms/2) * time.Millisecond longwait := time.Duration(2) * delay t := NewThrottleTimer("foo", delay) @@ -65,6 +66,21 @@ func TestThrottle(test *testing.T) { time.Sleep(longwait) assert.Equal(2, c.Count()) + // keep cancelling before it is ready + for i := 0; i < 10; i++ { + t.Set() + time.Sleep(shortwait) + t.Unset() + } + time.Sleep(longwait) + assert.Equal(2, c.Count()) + + // a few unsets do nothing... + for i := 0; i < 5; i++ { + t.Unset() + } + 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++ { @@ -74,5 +90,6 @@ func TestThrottle(test *testing.T) { time.Sleep(longwait) assert.Equal(5, c.Count()) - close(t.Ch) + stopped := t.Stop() + assert.True(stopped) } From 4ec7883891fa9700ce4b122252b8fc697df0bfca Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 11:21:01 +0100 Subject: [PATCH 26/61] Cleanup --- common/throttle_timer.go | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index e260e01b..705a12a1 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -1,7 +1,6 @@ package common import ( - "fmt" "time" ) @@ -64,7 +63,6 @@ func (t *ThrottleTimer) run() { // happen in this method. It is only called from the run goroutine // so we avoid any race conditions func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { - fmt.Printf("processInput: %d\n", cmd) switch cmd { case Set: if !t.isSet { @@ -77,9 +75,7 @@ func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { case Unset: if t.isSet { t.isSet = false - if !t.timer.Stop() { - <-t.timer.C - } + t.timer.Stop() } default: panic("unknown command!") From 0a8721113a67b3c05f58e12328a0fe0216811b0c Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 21:08:55 +0100 Subject: [PATCH 27/61] First pass of PR updates --- common/throttle_timer.go | 28 ++++++++++++++-------------- common/throttle_timer_test.go | 2 +- 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index 705a12a1..f2ce60b2 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -11,10 +11,11 @@ If a long continuous burst of .Set() calls happens, ThrottleTimer fires at most once every "dur". */ type ThrottleTimer struct { - Name string - Ch chan struct{} - input chan command - dur time.Duration + Name string + Ch <-chan struct{} + output chan<- struct{} + input chan command + dur time.Duration timer *time.Timer isSet bool @@ -29,12 +30,14 @@ const ( ) func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { + c := make(chan struct{}, 1) var t = &ThrottleTimer{ - Name: name, - Ch: make(chan struct{}, 1), - dur: dur, - input: make(chan command), - timer: time.NewTimer(dur), + Name: name, + Ch: c, + dur: dur, + output: c, + input: make(chan command), + timer: time.NewTimer(dur), } t.timer.Stop() go t.run() @@ -47,14 +50,12 @@ func (t *ThrottleTimer) run() { case cmd := <-t.input: // stop goroutine if the input says so if t.processInput(cmd) { - // TODO: do we want to close the channels??? - // close(t.Ch) - // close(t.input) + close(t.output) return } case <-t.timer.C: t.isSet = false - t.Ch <- struct{}{} + t.output <- struct{}{} } } } @@ -80,7 +81,6 @@ func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { default: panic("unknown command!") } - // return true return shutdown } diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index 014f9dcd..81b81703 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -10,7 +10,7 @@ import ( ) type thCounter struct { - input chan struct{} + input <-chan struct{} mtx sync.Mutex count int } From 1ac4c5dd6d007a708337e1ad2636e456e1e4b8db Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 21:20:30 +0100 Subject: [PATCH 28/61] Made throttle output non-blocking --- common/throttle_timer.go | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index f2ce60b2..069b6d84 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -30,7 +30,7 @@ const ( ) func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { - c := make(chan struct{}, 1) + c := make(chan struct{}) var t = &ThrottleTimer{ Name: name, Ch: c, @@ -54,12 +54,22 @@ func (t *ThrottleTimer) run() { return } case <-t.timer.C: - t.isSet = false - t.output <- struct{}{} + t.trySend() } } } +// trySend performs non-blocking send on t.output (t.Ch) +func (t *ThrottleTimer) trySend() { + select { + case t.output <- struct{}{}: + t.isSet = false + default: + // if we just want to drop, replace this with t.isSet = false + t.timer.Reset(t.dur) + } +} + // all modifications of the internal state of ThrottleTimer // happen in this method. It is only called from the run goroutine // so we avoid any race conditions From e430d3f8447d23b739840d5137ae75c37ff33a1d Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 21:51:23 +0100 Subject: [PATCH 29/61] One more attempt with a read-only channel --- common/throttle_timer.go | 33 ++++++++++++++++++--------------- common/throttle_timer_test.go | 2 +- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index 069b6d84..4a4b3003 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -11,11 +11,10 @@ If a long continuous burst of .Set() calls happens, ThrottleTimer fires at most once every "dur". */ type ThrottleTimer struct { - Name string - Ch <-chan struct{} - output chan<- struct{} - input chan command - dur time.Duration + Name string + Ch chan struct{} + input chan command + dur time.Duration timer *time.Timer isSet bool @@ -30,27 +29,31 @@ const ( ) func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { - c := make(chan struct{}) var t = &ThrottleTimer{ - Name: name, - Ch: c, - dur: dur, - output: c, - input: make(chan command), - timer: time.NewTimer(dur), + Name: name, + Ch: make(chan struct{}), + dur: dur, + input: make(chan command), + timer: time.NewTimer(dur), } t.timer.Stop() go t.run() return t } +// C is the proper way to listen to the timer output. +// t.Ch will be made private in the (near?) future +func (t *ThrottleTimer) C() <-chan struct{} { + return t.Ch +} + func (t *ThrottleTimer) run() { for { select { case cmd := <-t.input: // stop goroutine if the input says so if t.processInput(cmd) { - close(t.output) + close(t.Ch) return } case <-t.timer.C: @@ -59,10 +62,10 @@ func (t *ThrottleTimer) run() { } } -// trySend performs non-blocking send on t.output (t.Ch) +// trySend performs non-blocking send on t.Ch func (t *ThrottleTimer) trySend() { select { - case t.output <- struct{}{}: + case t.Ch <- struct{}{}: t.isSet = false default: // if we just want to drop, replace this with t.isSet = false diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index 81b81703..f6b5d1df 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -46,7 +46,7 @@ func TestThrottle(test *testing.T) { t := NewThrottleTimer("foo", delay) // start at 0 - c := &thCounter{input: t.Ch} + c := &thCounter{input: t.C()} assert.Equal(0, c.Count()) go c.Read() From 8b518fadb2f3eb928ce5d5a014b4087c5b31309a Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Wed, 6 Dec 2017 22:28:18 +0100 Subject: [PATCH 30/61] Don't close throttle channel, explain why --- common/throttle_timer.go | 2 +- common/throttle_timer_test.go | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index 4a4b3003..051d4437 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -52,8 +52,8 @@ func (t *ThrottleTimer) run() { select { case cmd := <-t.input: // stop goroutine if the input says so + // don't close channels, as closed channels mess up select reads if t.processInput(cmd) { - close(t.Ch) return } case <-t.timer.C: diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index f6b5d1df..7d96ac7c 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -31,6 +31,9 @@ func (c *thCounter) Count() int { // Read should run in a go-routine and // updates count by one every time a packet comes in func (c *thCounter) Read() { + // note, since this channel never closes, this will never end + // if thCounter was used in anything beyond trivial test cases. + // it would have to be smarter. for range c.input { c.Increment() } From 3779310c72c93173b9e87561281e697e7cdf9437 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Wed, 6 Dec 2017 18:48:39 -0600 Subject: [PATCH 31/61] return back output internal channel (way go does with Timer) --- common/throttle_timer.go | 47 +++++++++++++++++++++-------------- common/throttle_timer_test.go | 2 +- 2 files changed, 30 insertions(+), 19 deletions(-) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index 051d4437..ab2ad2e6 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -11,10 +11,11 @@ If a long continuous burst of .Set() calls happens, ThrottleTimer fires at most once every "dur". */ type ThrottleTimer struct { - Name string - Ch chan struct{} - input chan command - dur time.Duration + Name string + Ch <-chan struct{} + input chan command + output chan<- struct{} + dur time.Duration timer *time.Timer isSet bool @@ -28,25 +29,22 @@ const ( Quit ) +// NewThrottleTimer creates a new ThrottleTimer. func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { + c := make(chan struct{}) var t = &ThrottleTimer{ - Name: name, - Ch: make(chan struct{}), - dur: dur, - input: make(chan command), - timer: time.NewTimer(dur), + Name: name, + Ch: c, + dur: dur, + input: make(chan command), + output: c, + timer: time.NewTimer(dur), } t.timer.Stop() go t.run() return t } -// C is the proper way to listen to the timer output. -// t.Ch will be made private in the (near?) future -func (t *ThrottleTimer) C() <-chan struct{} { - return t.Ch -} - func (t *ThrottleTimer) run() { for { select { @@ -65,7 +63,7 @@ func (t *ThrottleTimer) run() { // trySend performs non-blocking send on t.Ch func (t *ThrottleTimer) trySend() { select { - case t.Ch <- struct{}{}: + case t.output <- struct{}{}: t.isSet = false default: // if we just want to drop, replace this with t.isSet = false @@ -105,8 +103,21 @@ func (t *ThrottleTimer) Unset() { t.input <- Unset } -// For ease of .Stop()'ing services before .Start()'ing them, -// we ignore .Stop()'s on nil ThrottleTimers +// Stop prevents the ThrottleTimer from firing. It always returns true. Stop does not +// close the channel, to prevent a read from the channel succeeding +// incorrectly. +// +// To prevent a timer created with NewThrottleTimer from firing after a call to +// Stop, check the return value and drain the channel. +// +// For example, assuming the program has not received from t.C already: +// +// if !t.Stop() { +// <-t.C +// } +// +// For ease of stopping services before starting them, we ignore Stop on nil +// ThrottleTimers. func (t *ThrottleTimer) Stop() bool { if t == nil { return false diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index 7d96ac7c..a1b6606f 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -49,7 +49,7 @@ func TestThrottle(test *testing.T) { t := NewThrottleTimer("foo", delay) // start at 0 - c := &thCounter{input: t.C()} + c := &thCounter{input: t.Ch} assert.Equal(0, c.Count()) go c.Read() From 887d766c86f1f217653915a2042374972c8f38ae Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 7 Dec 2017 10:15:38 +0100 Subject: [PATCH 32/61] Refactored RepeatTimer, tests hang --- common/repeat_timer.go | 129 +++++++++++++++++++++--------------- common/repeat_timer_test.go | 4 +- common/throttle_timer.go | 16 ++--- 3 files changed, 85 insertions(+), 64 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index d7d9154d..0f650113 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -1,7 +1,7 @@ package common import ( - "sync" + "fmt" "time" ) @@ -11,54 +11,40 @@ 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 + Name string + Ch <-chan time.Time + output chan<- time.Time + input chan repeatCommand - mtx sync.Mutex - name string - ticker *time.Ticker - quit chan struct{} - wg *sync.WaitGroup - dur time.Duration + dur time.Duration + timer *time.Timer } +type repeatCommand int32 + +const ( + Reset repeatCommand = iota + RQuit +) + func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer { + c := make(chan time.Time) var t = &RepeatTimer{ - Ch: make(chan time.Time), - ticker: time.NewTicker(dur), - quit: make(chan struct{}), - wg: new(sync.WaitGroup), - name: name, - dur: dur, - } - t.wg.Add(1) - go t.fireRoutine(t.ticker) - return t -} + Name: name, + Ch: c, + output: c, + input: make(chan repeatCommand), -func (t *RepeatTimer) fireRoutine(ticker *time.Ticker) { - 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() - return - } + timer: time.NewTimer(dur), + dur: dur, } + go t.run() + return t } // 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) + t.input <- Reset } // For ease of .Stop()'ing services before .Start()'ing them, @@ -67,20 +53,55 @@ func (t *RepeatTimer) Stop() bool { if t == nil { return false } - t.mtx.Lock() // Lock - defer t.mtx.Unlock() - - exists := t.ticker != nil - if exists { - t.ticker.Stop() // does not close the channel - select { - 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 + t.input <- RQuit + return true +} + +func (t *RepeatTimer) run() { + for { + fmt.Println("for") + select { + case cmd := <-t.input: + // stop goroutine if the input says so + // don't close channels, as closed channels mess up select reads + if t.processInput(cmd) { + t.timer.Stop() + return + } + case <-t.timer.C: + fmt.Println("tick") + // send if not blocked, then start the next tick + // for blocking send, just + // t.output <- time.Now() + t.trySend() + t.timer.Reset(t.dur) + } + } +} + +// trySend performs non-blocking send on t.Ch +func (t *RepeatTimer) trySend() { + // TODO: this was blocking in previous version (t.Ch <- t_) + // should I use that behavior unstead of unblocking as per throttle? + select { + case t.output <- time.Now(): + default: + } +} + +// all modifications of the internal state of ThrottleTimer +// happen in this method. It is only called from the run goroutine +// so we avoid any race conditions +func (t *RepeatTimer) processInput(cmd repeatCommand) (shutdown bool) { + fmt.Printf("process: %d\n", cmd) + switch cmd { + case Reset: + t.timer.Reset(t.dur) + case RQuit: + t.timer.Stop() + shutdown = true + default: + panic("unknown command!") + } + return shutdown } diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 87f34b95..d66cd315 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -10,7 +10,7 @@ import ( ) type rCounter struct { - input chan time.Time + input <-chan time.Time mtx sync.Mutex count int } @@ -74,5 +74,5 @@ func TestRepeat(test *testing.T) { assert.Equal(6, c.Count()) // close channel to stop counter - close(t.Ch) + t.Stop() } diff --git a/common/throttle_timer.go b/common/throttle_timer.go index ab2ad2e6..c148d990 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -13,7 +13,7 @@ at most once every "dur". type ThrottleTimer struct { Name string Ch <-chan struct{} - input chan command + input chan throttleCommand output chan<- struct{} dur time.Duration @@ -21,12 +21,12 @@ type ThrottleTimer struct { isSet bool } -type command int32 +type throttleCommand int32 const ( - Set command = iota + Set throttleCommand = iota Unset - Quit + TQuit ) // NewThrottleTimer creates a new ThrottleTimer. @@ -36,7 +36,7 @@ func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { Name: name, Ch: c, dur: dur, - input: make(chan command), + input: make(chan throttleCommand), output: c, timer: time.NewTimer(dur), } @@ -74,14 +74,14 @@ func (t *ThrottleTimer) trySend() { // all modifications of the internal state of ThrottleTimer // happen in this method. It is only called from the run goroutine // so we avoid any race conditions -func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { +func (t *ThrottleTimer) processInput(cmd throttleCommand) (shutdown bool) { switch cmd { case Set: if !t.isSet { t.isSet = true t.timer.Reset(t.dur) } - case Quit: + case TQuit: shutdown = true fallthrough case Unset: @@ -122,6 +122,6 @@ func (t *ThrottleTimer) Stop() bool { if t == nil { return false } - t.input <- Quit + t.input <- TQuit return true } From 8797197cdfc9920e2dbce274c8aba8c09b15f86f Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 7 Dec 2017 10:36:03 +0100 Subject: [PATCH 33/61] No more blocking on multiple Stop() --- common/repeat_timer.go | 33 +++++++++++++++++---------------- common/repeat_timer_test.go | 2 +- common/throttle_timer.go | 8 +++++--- common/throttle_timer_test.go | 5 +++++ 4 files changed, 28 insertions(+), 20 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 0f650113..734c2d32 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -16,8 +16,9 @@ type RepeatTimer struct { output chan<- time.Time input chan repeatCommand - dur time.Duration - timer *time.Timer + dur time.Duration + timer *time.Timer + stopped bool } type repeatCommand int32 @@ -50,43 +51,42 @@ func (t *RepeatTimer) Reset() { // 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 { + if t == nil || t.stopped { return false } t.input <- RQuit + t.stopped = true return true } func (t *RepeatTimer) run() { - for { - fmt.Println("for") + done := false + for !done { select { case cmd := <-t.input: // stop goroutine if the input says so // don't close channels, as closed channels mess up select reads - if t.processInput(cmd) { - t.timer.Stop() - return - } + done = t.processInput(cmd) case <-t.timer.C: - fmt.Println("tick") // send if not blocked, then start the next tick - // for blocking send, just - // t.output <- time.Now() t.trySend() t.timer.Reset(t.dur) } } + fmt.Println("end run") } // trySend performs non-blocking send on t.Ch func (t *RepeatTimer) trySend() { // TODO: this was blocking in previous version (t.Ch <- t_) // should I use that behavior unstead of unblocking as per throttle? - select { - case t.output <- time.Now(): - default: - } + + // select { + // case t.output <- time.Now(): + // default: + // } + + t.output <- time.Now() } // all modifications of the internal state of ThrottleTimer @@ -98,6 +98,7 @@ func (t *RepeatTimer) processInput(cmd repeatCommand) (shutdown bool) { case Reset: t.timer.Reset(t.dur) case RQuit: + fmt.Println("got quit") t.timer.Stop() shutdown = true default: diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index d66cd315..15ca32c3 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -73,6 +73,6 @@ func TestRepeat(test *testing.T) { time.Sleep(delay(7)) assert.Equal(6, c.Count()) - // close channel to stop counter + // extra calls to stop don't block t.Stop() } diff --git a/common/throttle_timer.go b/common/throttle_timer.go index c148d990..0e54f102 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -17,8 +17,9 @@ type ThrottleTimer struct { output chan<- struct{} dur time.Duration - timer *time.Timer - isSet bool + timer *time.Timer + isSet bool + stopped bool } type throttleCommand int32 @@ -82,6 +83,7 @@ func (t *ThrottleTimer) processInput(cmd throttleCommand) (shutdown bool) { t.timer.Reset(t.dur) } case TQuit: + t.stopped = true shutdown = true fallthrough case Unset: @@ -119,7 +121,7 @@ func (t *ThrottleTimer) Unset() { // For ease of stopping services before starting them, we ignore Stop on nil // ThrottleTimers. func (t *ThrottleTimer) Stop() bool { - if t == nil { + if t == nil || t.stopped { return false } t.input <- TQuit diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index a1b6606f..2a81bb02 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -95,4 +95,9 @@ func TestThrottle(test *testing.T) { stopped := t.Stop() assert.True(stopped) + time.Sleep(longwait) + assert.Equal(5, c.Count()) + + // extra calls to stop don't block + t.Stop() } From cc7a87e27caa55ca84e984d1d081b09eeb16ffe6 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Thu, 7 Dec 2017 11:22:54 +0100 Subject: [PATCH 34/61] Use Ticker in Repeat again to avoid drift --- common/repeat_timer.go | 34 ++++++++++++++-------------------- common/repeat_timer_test.go | 6 +++--- 2 files changed, 17 insertions(+), 23 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 734c2d32..b3eb107d 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -1,7 +1,6 @@ package common import ( - "fmt" "time" ) @@ -17,7 +16,7 @@ type RepeatTimer struct { input chan repeatCommand dur time.Duration - timer *time.Timer + ticker *time.Ticker stopped bool } @@ -36,8 +35,8 @@ func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer { output: c, input: make(chan repeatCommand), - timer: time.NewTimer(dur), - dur: dur, + dur: dur, + ticker: time.NewTicker(dur), } go t.run() return t @@ -51,6 +50,7 @@ func (t *RepeatTimer) Reset() { // For ease of .Stop()'ing services before .Start()'ing them, // we ignore .Stop()'s on nil RepeatTimers. func (t *RepeatTimer) Stop() bool { + // use t.stopped to gracefully handle many Stop() without blocking if t == nil || t.stopped { return false } @@ -67,39 +67,33 @@ func (t *RepeatTimer) run() { // stop goroutine if the input says so // don't close channels, as closed channels mess up select reads done = t.processInput(cmd) - case <-t.timer.C: - // send if not blocked, then start the next tick + case <-t.ticker.C: t.trySend() - t.timer.Reset(t.dur) } } - fmt.Println("end run") } // trySend performs non-blocking send on t.Ch func (t *RepeatTimer) trySend() { - // TODO: this was blocking in previous version (t.Ch <- t_) + // NOTE: this was blocking in previous version (t.Ch <- t_) // should I use that behavior unstead of unblocking as per throttle? - - // select { - // case t.output <- time.Now(): - // default: - // } - - t.output <- time.Now() + // probably not: https://golang.org/src/time/sleep.go#L132 + select { + case t.output <- time.Now(): + default: + } } // all modifications of the internal state of ThrottleTimer // happen in this method. It is only called from the run goroutine // so we avoid any race conditions func (t *RepeatTimer) processInput(cmd repeatCommand) (shutdown bool) { - fmt.Printf("process: %d\n", cmd) switch cmd { case Reset: - t.timer.Reset(t.dur) + t.ticker.Stop() + t.ticker = time.NewTicker(t.dur) case RQuit: - fmt.Println("got quit") - t.timer.Stop() + t.ticker.Stop() shutdown = true default: panic("unknown command!") diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 15ca32c3..db53aa61 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -39,11 +39,11 @@ func (c *rCounter) Read() { func TestRepeat(test *testing.T) { assert := asrt.New(test) - dur := time.Duration(50) * time.Millisecond + dur := time.Duration(100) * time.Millisecond short := time.Duration(20) * time.Millisecond // delay waits for cnt durations, an a little extra delay := func(cnt int) time.Duration { - return time.Duration(cnt)*dur + time.Duration(5)*time.Millisecond + return time.Duration(cnt)*dur + time.Duration(10)*time.Millisecond } t := NewRepeatTimer("bar", dur) @@ -70,7 +70,7 @@ func TestRepeat(test *testing.T) { // after a stop, nothing more is sent stopped := t.Stop() assert.True(stopped) - time.Sleep(delay(7)) + time.Sleep(delay(2)) assert.Equal(6, c.Count()) // extra calls to stop don't block From ec4adf21e0451f3fb7da33932d6cac168ddeaa93 Mon Sep 17 00:00:00 2001 From: Ethan Frey Date: Fri, 8 Dec 2017 10:07:04 +0100 Subject: [PATCH 35/61] Cleanup from PR comments --- common/repeat_timer.go | 20 ++++++++++---------- common/throttle_timer.go | 4 ++-- common/throttle_timer_test.go | 3 --- 3 files changed, 12 insertions(+), 15 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index b3eb107d..77f73603 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -20,7 +20,7 @@ type RepeatTimer struct { stopped bool } -type repeatCommand int32 +type repeatCommand int8 const ( Reset repeatCommand = iota @@ -67,21 +67,21 @@ func (t *RepeatTimer) run() { // stop goroutine if the input says so // don't close channels, as closed channels mess up select reads done = t.processInput(cmd) - case <-t.ticker.C: - t.trySend() + case tick := <-t.ticker.C: + t.trySend(tick) } } } // trySend performs non-blocking send on t.Ch -func (t *RepeatTimer) trySend() { +func (t *RepeatTimer) trySend(tick time.Time) { // NOTE: this was blocking in previous version (t.Ch <- t_) - // should I use that behavior unstead of unblocking as per throttle? - // probably not: https://golang.org/src/time/sleep.go#L132 - select { - case t.output <- time.Now(): - default: - } + // probably better not: https://golang.org/src/time/sleep.go#L132 + t.output <- tick + // select { + // case t.output <- tick: + // default: + // } } // all modifications of the internal state of ThrottleTimer diff --git a/common/throttle_timer.go b/common/throttle_timer.go index 0e54f102..a5bd6ded 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -22,7 +22,7 @@ type ThrottleTimer struct { stopped bool } -type throttleCommand int32 +type throttleCommand int8 const ( Set throttleCommand = iota @@ -83,7 +83,6 @@ func (t *ThrottleTimer) processInput(cmd throttleCommand) (shutdown bool) { t.timer.Reset(t.dur) } case TQuit: - t.stopped = true shutdown = true fallthrough case Unset: @@ -125,5 +124,6 @@ func (t *ThrottleTimer) Stop() bool { return false } t.input <- TQuit + t.stopped = true return true } diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index 2a81bb02..94ec1b43 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -95,9 +95,6 @@ func TestThrottle(test *testing.T) { stopped := t.Stop() assert.True(stopped) - time.Sleep(longwait) - assert.Equal(5, c.Count()) - // extra calls to stop don't block t.Stop() } From ff2fd63bf7db6373e5fb0c1d311c6a139b99dfe0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 8 Dec 2017 11:17:07 -0600 Subject: [PATCH 36/61] rename trySend to send --- common/repeat_timer.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 77f73603..23faf74a 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -68,20 +68,20 @@ func (t *RepeatTimer) run() { // don't close channels, as closed channels mess up select reads done = t.processInput(cmd) case tick := <-t.ticker.C: - t.trySend(tick) + t.send(tick) } } } -// trySend performs non-blocking send on t.Ch -func (t *RepeatTimer) trySend(tick time.Time) { - // NOTE: this was blocking in previous version (t.Ch <- t_) - // probably better not: https://golang.org/src/time/sleep.go#L132 - t.output <- tick +// send performs blocking send on t.Ch +func (t *RepeatTimer) send(tick time.Time) { + // XXX: possibly it is better to not block: + // https://golang.org/src/time/sleep.go#L132 // select { // case t.output <- tick: // default: // } + t.output <- tick } // all modifications of the internal state of ThrottleTimer From cb4ba522ef643073c1b1ae372ef0a5e32078cb5f Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Sat, 9 Dec 2017 23:05:13 -0600 Subject: [PATCH 37/61] add String method to Query interface Required for https://github.com/tendermint/tendermint/issues/945 --- pubsub/pubsub.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pubsub/pubsub.go b/pubsub/pubsub.go index 52b8361f..27f15cbe 100644 --- a/pubsub/pubsub.go +++ b/pubsub/pubsub.go @@ -38,6 +38,7 @@ type cmd struct { // Query defines an interface for a query to be used for subscribing. type Query interface { Matches(tags map[string]interface{}) bool + String() string } // Server allows clients to subscribe/unsubscribe for messages, publishing From e4ef2835f0081c2ece83b9c1f777cf071f956e81 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Sat, 9 Dec 2017 23:35:14 -0600 Subject: [PATCH 38/61] return error if client already subscribed --- pubsub/pubsub.go | 68 ++++++++++++++++++++++++++++++++++--------- pubsub/pubsub_test.go | 32 +++++++++++++++----- 2 files changed, 80 insertions(+), 20 deletions(-) diff --git a/pubsub/pubsub.go b/pubsub/pubsub.go index 27f15cbe..54a4b8ae 100644 --- a/pubsub/pubsub.go +++ b/pubsub/pubsub.go @@ -13,6 +13,8 @@ package pubsub import ( "context" + "errors" + "sync" cmn "github.com/tendermint/tmlibs/common" ) @@ -48,6 +50,9 @@ type Server struct { cmds chan cmd cmdsCap int + + mtx sync.RWMutex + subscriptions map[string]map[string]struct{} // subscriber -> query -> struct{} } // Option sets a parameter for the server. @@ -57,7 +62,9 @@ type Option func(*Server) // for a detailed description of how to configure buffering. If no options are // provided, the resulting server's queue is unbuffered. func NewServer(options ...Option) *Server { - s := &Server{} + s := &Server{ + subscriptions: make(map[string]map[string]struct{}), + } s.BaseService = *cmn.NewBaseService(nil, "PubSub", s) for _, option := range options { @@ -83,17 +90,33 @@ func BufferCapacity(cap int) Option { } // BufferCapacity returns capacity of the internal server's queue. -func (s Server) BufferCapacity() int { +func (s *Server) BufferCapacity() int { return s.cmdsCap } // Subscribe creates a subscription for the given client. It accepts a channel -// on which messages matching the given query can be received. If the -// subscription already exists, the old channel will be closed. An error will -// be returned to the caller if the context is canceled. +// on which messages matching the given query can be received. An error will be +// returned to the caller if the context is canceled or if subscription already +// exist for pair clientID and query. func (s *Server) Subscribe(ctx context.Context, clientID string, query Query, out chan<- interface{}) error { + s.mtx.RLock() + clientSubscriptions, ok := s.subscriptions[clientID] + if ok { + _, ok = clientSubscriptions[query.String()] + } + s.mtx.RUnlock() + if ok { + return errors.New("already subscribed") + } + select { case s.cmds <- cmd{op: sub, clientID: clientID, query: query, ch: out}: + s.mtx.Lock() + if _, ok = s.subscriptions[clientID]; !ok { + s.subscriptions[clientID] = make(map[string]struct{}) + } + s.subscriptions[clientID][query.String()] = struct{}{} + s.mtx.Unlock() return nil case <-ctx.Done(): return ctx.Err() @@ -101,10 +124,24 @@ func (s *Server) Subscribe(ctx context.Context, clientID string, query Query, ou } // Unsubscribe removes the subscription on the given query. An error will be -// returned to the caller if the context is canceled. +// returned to the caller if the context is canceled or if subscription does +// not exist. func (s *Server) Unsubscribe(ctx context.Context, clientID string, query Query) error { + s.mtx.RLock() + clientSubscriptions, ok := s.subscriptions[clientID] + if ok { + _, ok = clientSubscriptions[query.String()] + } + s.mtx.RUnlock() + if !ok { + return errors.New("subscription not found") + } + select { case s.cmds <- cmd{op: unsub, clientID: clientID, query: query}: + s.mtx.Lock() + delete(clientSubscriptions, query.String()) + s.mtx.Unlock() return nil case <-ctx.Done(): return ctx.Err() @@ -112,10 +149,20 @@ func (s *Server) Unsubscribe(ctx context.Context, clientID string, query Query) } // UnsubscribeAll removes all client subscriptions. An error will be returned -// to the caller if the context is canceled. +// to the caller if the context is canceled or if subscription does not exist. func (s *Server) UnsubscribeAll(ctx context.Context, clientID string) error { + s.mtx.RLock() + _, ok := s.subscriptions[clientID] + s.mtx.RUnlock() + if !ok { + return errors.New("subscription not found") + } + select { case s.cmds <- cmd{op: unsub, clientID: clientID}: + s.mtx.Lock() + delete(s.subscriptions, clientID) + s.mtx.Unlock() return nil case <-ctx.Done(): return ctx.Err() @@ -187,13 +234,8 @@ loop: func (state *state) add(clientID string, q Query, ch chan<- interface{}) { // add query if needed - if clientToChannelMap, ok := state.queries[q]; !ok { + if _, ok := state.queries[q]; !ok { state.queries[q] = make(map[string]chan<- interface{}) - } else { - // check if already subscribed - if oldCh, ok := clientToChannelMap[clientID]; ok { - close(oldCh) - } } // create subscription diff --git a/pubsub/pubsub_test.go b/pubsub/pubsub_test.go index 7bf7b41f..84b6aa21 100644 --- a/pubsub/pubsub_test.go +++ b/pubsub/pubsub_test.go @@ -86,14 +86,11 @@ func TestClientSubscribesTwice(t *testing.T) { ch2 := make(chan interface{}, 1) err = s.Subscribe(ctx, clientID, q, ch2) - require.NoError(t, err) - - _, ok := <-ch1 - assert.False(t, ok) + require.Error(t, err) err = s.PublishWithTags(ctx, "Spider-Man", map[string]interface{}{"tm.events.type": "NewBlock"}) require.NoError(t, err) - assertReceive(t, "Spider-Man", ch2) + assertReceive(t, "Spider-Man", ch1) } func TestUnsubscribe(t *testing.T) { @@ -117,6 +114,27 @@ func TestUnsubscribe(t *testing.T) { assert.False(t, ok) } +func TestResubscribe(t *testing.T) { + s := pubsub.NewServer() + s.SetLogger(log.TestingLogger()) + s.Start() + defer s.Stop() + + ctx := context.Background() + ch := make(chan interface{}) + err := s.Subscribe(ctx, clientID, query.Empty{}, ch) + require.NoError(t, err) + err = s.Unsubscribe(ctx, clientID, query.Empty{}) + require.NoError(t, err) + ch = make(chan interface{}) + err = s.Subscribe(ctx, clientID, query.Empty{}, ch) + require.NoError(t, err) + + err = s.Publish(ctx, "Cable") + require.NoError(t, err) + assertReceive(t, "Cable", ch) +} + func TestUnsubscribeAll(t *testing.T) { s := pubsub.NewServer() s.SetLogger(log.TestingLogger()) @@ -125,9 +143,9 @@ func TestUnsubscribeAll(t *testing.T) { ctx := context.Background() ch1, ch2 := make(chan interface{}, 1), make(chan interface{}, 1) - err := s.Subscribe(ctx, clientID, query.Empty{}, ch1) + err := s.Subscribe(ctx, clientID, query.MustParse("tm.events.type='NewBlock'"), ch1) require.NoError(t, err) - err = s.Subscribe(ctx, clientID, query.Empty{}, ch2) + err = s.Subscribe(ctx, clientID, query.MustParse("tm.events.type='NewBlockHeader'"), ch2) require.NoError(t, err) err = s.UnsubscribeAll(ctx, clientID) From f39b575503b80cf22753f70ddc2956925b7b1ac4 Mon Sep 17 00:00:00 2001 From: Zach Ramsay Date: Tue, 12 Dec 2017 16:55:41 +0000 Subject: [PATCH 39/61] remove deprecated --root flag --- cli/setup.go | 27 +++++++-------------------- cli/setup_test.go | 20 ++++++++++---------- 2 files changed, 17 insertions(+), 30 deletions(-) diff --git a/cli/setup.go b/cli/setup.go index 78151015..29547759 100644 --- a/cli/setup.go +++ b/cli/setup.go @@ -14,7 +14,6 @@ import ( ) const ( - RootFlag = "root" HomeFlag = "home" TraceFlag = "trace" OutputFlag = "output" @@ -28,14 +27,9 @@ type Executable interface { } // PrepareBaseCmd is meant for tendermint and other servers -func PrepareBaseCmd(cmd *cobra.Command, envPrefix, defautRoot string) Executor { +func PrepareBaseCmd(cmd *cobra.Command, envPrefix, defaultHome string) Executor { cobra.OnInitialize(func() { initEnv(envPrefix) }) - cmd.PersistentFlags().StringP(RootFlag, "r", defautRoot, "DEPRECATED. Use --home") - // -h is already reserved for --help as part of the cobra framework - // do you want to try something else?? - // also, default must be empty, so we can detect this unset and fall back - // to --root / TM_ROOT / TMROOT - cmd.PersistentFlags().String(HomeFlag, "", "root directory for config and data") + cmd.PersistentFlags().StringP(HomeFlag, "", defaultHome, "directory for config and data") cmd.PersistentFlags().Bool(TraceFlag, false, "print out full stack trace on errors") cmd.PersistentPreRunE = concatCobraCmdFuncs(bindFlagsLoadViper, cmd.PersistentPreRunE) return Executor{cmd, os.Exit} @@ -45,11 +39,11 @@ func PrepareBaseCmd(cmd *cobra.Command, envPrefix, defautRoot string) Executor { // // This adds --encoding (hex, btc, base64) and --output (text, json) to // the command. These only really make sense in interactive commands. -func PrepareMainCmd(cmd *cobra.Command, envPrefix, defautRoot string) Executor { +func PrepareMainCmd(cmd *cobra.Command, envPrefix, defaultHome string) Executor { cmd.PersistentFlags().StringP(EncodingFlag, "e", "hex", "Binary encoding (hex|b64|btc)") cmd.PersistentFlags().StringP(OutputFlag, "o", "text", "Output format (text|json)") cmd.PersistentPreRunE = concatCobraCmdFuncs(setEncoding, validateOutput, cmd.PersistentPreRunE) - return PrepareBaseCmd(cmd, envPrefix, defautRoot) + return PrepareBaseCmd(cmd, envPrefix, defaultHome) } // initEnv sets to use ENV variables if set. @@ -136,17 +130,10 @@ func bindFlagsLoadViper(cmd *cobra.Command, args []string) error { return err } - // rootDir is command line flag, env variable, or default $HOME/.tlc - // NOTE: we support both --root and --home for now, but eventually only --home - // Also ensure we set the correct rootDir under HomeFlag so we dont need to - // repeat this logic elsewhere. - rootDir := viper.GetString(HomeFlag) - if rootDir == "" { - rootDir = viper.GetString(RootFlag) - viper.Set(HomeFlag, rootDir) - } + homeDir := viper.GetString(HomeFlag) + viper.Set(HomeFlag, homeDir) viper.SetConfigName("config") // name of config file (without extension) - viper.AddConfigPath(rootDir) // search root directory + viper.AddConfigPath(homeDir) // search root directory // If a config file is found, read it in. if err := viper.ReadInConfig(); err == nil { diff --git a/cli/setup_test.go b/cli/setup_test.go index 692da26d..2f085f7d 100644 --- a/cli/setup_test.go +++ b/cli/setup_test.go @@ -74,16 +74,16 @@ func TestSetupConfig(t *testing.T) { // setting on the command line {[]string{"--boo", "haha"}, nil, "haha", ""}, {[]string{"--two-words", "rocks"}, nil, "", "rocks"}, - {[]string{"--root", conf1}, nil, cval1, ""}, + {[]string{"--home", conf1}, nil, cval1, ""}, // test both variants of the prefix - {nil, map[string]string{"RD_BOO": "bang"}, "bang", ""}, - {nil, map[string]string{"RD_TWO_WORDS": "fly"}, "", "fly"}, - {nil, map[string]string{"RDTWO_WORDS": "fly"}, "", "fly"}, - {nil, map[string]string{"RD_ROOT": conf1}, cval1, ""}, - {nil, map[string]string{"RDROOT": conf2}, cval2, "WORD"}, - {nil, map[string]string{"RDHOME": conf1}, cval1, ""}, + //{nil, map[string]string{"RD_BOO": "bang"}, "bang", ""}, + //{nil, map[string]string{"RD_TWO_WORDS": "fly"}, "", "fly"}, + //{nil, map[string]string{"RDTWO_WORDS": "fly"}, "", "fly"}, + //{nil, map[string]string{"RD_ROOT": conf1}, cval1, ""}, + //{nil, map[string]string{"RDROOT": conf2}, cval2, "WORD"}, + //{nil, map[string]string{"RDHOME": conf1}, cval1, ""}, // and when both are set??? HOME wins every time! - {[]string{"--root", conf1}, map[string]string{"RDHOME": conf2}, cval2, "WORD"}, + {[]string{"--home", conf1}, map[string]string{"RDHOME": conf2}, cval2, "WORD"}, } for idx, tc := range cases { @@ -156,10 +156,10 @@ func TestSetupUnmarshal(t *testing.T) { {nil, nil, c("", 0)}, // setting on the command line {[]string{"--name", "haha"}, nil, c("haha", 0)}, - {[]string{"--root", conf1}, nil, c(cval1, 0)}, + {[]string{"--home", conf1}, nil, c(cval1, 0)}, // test both variants of the prefix {nil, map[string]string{"MR_AGE": "56"}, c("", 56)}, - {nil, map[string]string{"MR_ROOT": conf1}, c(cval1, 0)}, + //{nil, map[string]string{"MR_ROOT": conf1}, c(cval1, 0)}, {[]string{"--age", "17"}, map[string]string{"MRHOME": conf2}, c(cval2, 17)}, } From 541780c6dff65a2d3554ac297ae2c7e61d8217f6 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 12 Dec 2017 23:23:49 -0600 Subject: [PATCH 40/61] uncomment tests --- cli/setup_test.go | 20 +++++++------------- 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/cli/setup_test.go b/cli/setup_test.go index 2f085f7d..e0fd75d8 100644 --- a/cli/setup_test.go +++ b/cli/setup_test.go @@ -57,12 +57,9 @@ func TestSetupEnv(t *testing.T) { func TestSetupConfig(t *testing.T) { // we pre-create two config files we can refer to in the rest of // the test cases. - cval1, cval2 := "fubble", "wubble" + cval1 := "fubble" conf1, err := WriteDemoConfig(map[string]string{"boo": cval1}) require.Nil(t, err) - // make sure it handles dashed-words in the config, and ignores random info - conf2, err := WriteDemoConfig(map[string]string{"boo": cval2, "foo": "bar", "two-words": "WORD"}) - require.Nil(t, err) cases := []struct { args []string @@ -76,14 +73,11 @@ func TestSetupConfig(t *testing.T) { {[]string{"--two-words", "rocks"}, nil, "", "rocks"}, {[]string{"--home", conf1}, nil, cval1, ""}, // test both variants of the prefix - //{nil, map[string]string{"RD_BOO": "bang"}, "bang", ""}, - //{nil, map[string]string{"RD_TWO_WORDS": "fly"}, "", "fly"}, - //{nil, map[string]string{"RDTWO_WORDS": "fly"}, "", "fly"}, - //{nil, map[string]string{"RD_ROOT": conf1}, cval1, ""}, - //{nil, map[string]string{"RDROOT": conf2}, cval2, "WORD"}, - //{nil, map[string]string{"RDHOME": conf1}, cval1, ""}, - // and when both are set??? HOME wins every time! - {[]string{"--home", conf1}, map[string]string{"RDHOME": conf2}, cval2, "WORD"}, + {nil, map[string]string{"RD_BOO": "bang"}, "bang", ""}, + {nil, map[string]string{"RD_TWO_WORDS": "fly"}, "", "fly"}, + {nil, map[string]string{"RDTWO_WORDS": "fly"}, "", "fly"}, + {nil, map[string]string{"RD_HOME": conf1}, cval1, ""}, + {nil, map[string]string{"RDHOME": conf1}, cval1, ""}, } for idx, tc := range cases { @@ -159,7 +153,7 @@ func TestSetupUnmarshal(t *testing.T) { {[]string{"--home", conf1}, nil, c(cval1, 0)}, // test both variants of the prefix {nil, map[string]string{"MR_AGE": "56"}, c("", 56)}, - //{nil, map[string]string{"MR_ROOT": conf1}, c(cval1, 0)}, + {nil, map[string]string{"MR_HOME": conf1}, c(cval1, 0)}, {[]string{"--age", "17"}, map[string]string{"MRHOME": conf2}, c(cval2, 17)}, } From 29471d75cb50eb4cea5878b8bd1be25e8150564c Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Wed, 13 Dec 2017 22:53:02 -0700 Subject: [PATCH 41/61] common: no more relying on math/rand.DefaultSource Fixes https://github.com/tendermint/tmlibs/issues/99 Updates https://github.com/tendermint/tendermint/issues/973 Removed usages of math/rand.DefaultSource in favour of our own source that's seeded with a completely random source and is safe for use in concurrent in multiple goroutines. Also extend some functionality that the stdlib exposes such as * RandPerm * RandIntn * RandInt31 * RandInt63 Also added an integration test whose purpose is to be run as a consistency check to ensure that our results never repeat hence that our internal PRNG is uniquely seeded each time. This integration test can be triggered by setting environment variable: `TENDERMINT_INTEGRATION_TESTS=true` for example ```shell TENDERMINT_INTEGRATION_TESTS=true go test ``` --- common/bit_array.go | 7 ++- common/random.go | 89 +++++++++++++++++++++++++--------- common/random_test.go | 108 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 178 insertions(+), 26 deletions(-) create mode 100644 common/random_test.go diff --git a/common/bit_array.go b/common/bit_array.go index 5590fe61..848763b4 100644 --- a/common/bit_array.go +++ b/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 { diff --git a/common/random.go b/common/random.go index 73bd1635..f0d169e0 100644 --- a/common/random.go +++ b/common/random.go @@ -3,6 +3,7 @@ package common import ( crand "crypto/rand" "math/rand" + "sync" "time" ) @@ -10,6 +11,11 @@ const ( strChars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" // 62 characters ) +var rng struct { + sync.Mutex + *rand.Rand +} + func init() { b := cRandBytes(8) var seed uint64 @@ -17,7 +23,7 @@ func init() { seed |= uint64(b[i]) seed <<= 8 } - rand.Seed(int64(seed)) + rng.Rand = rand.New(rand.NewSource(int64(seed))) } // Constructs an alphanumeric string of given length. @@ -25,7 +31,7 @@ func RandStr(length int) string { chars := []byte{} MAIN_LOOP: for { - val := rand.Int63() + val := rng.Int63() for i := 0; i < 10; i++ { v := int(val & 0x3f) // rightmost 6 bits if v >= 62 { // only 62 characters in strChars @@ -45,72 +51,98 @@ MAIN_LOOP: } func RandUint16() uint16 { - return uint16(rand.Uint32() & (1<<16 - 1)) + return uint16(RandUint32() & (1<<16 - 1)) } func RandUint32() uint32 { - return rand.Uint32() + rng.Lock() + u32 := rng.Uint32() + rng.Unlock() + return u32 } func RandUint64() uint64 { - return uint64(rand.Uint32())<<32 + uint64(rand.Uint32()) + return uint64(RandUint32())<<32 + uint64(RandUint32()) } func RandUint() uint { - return uint(rand.Int()) + rng.Lock() + i := rng.Int() + rng.Unlock() + return uint(i) } func RandInt16() int16 { - return int16(rand.Uint32() & (1<<16 - 1)) + return int16(RandUint32() & (1<<16 - 1)) } func RandInt32() int32 { - return int32(rand.Uint32()) + return int32(RandUint32()) } func RandInt64() int64 { - return int64(rand.Uint32())<<32 + int64(rand.Uint32()) + return int64(RandUint64()) } func RandInt() int { - return rand.Int() + rng.Lock() + i := rng.Int() + rng.Unlock() + return i +} + +func RandInt31() int32 { + rng.Lock() + i31 := rng.Int31() + rng.Unlock() + return i31 +} + +func RandInt63() int64 { + rng.Lock() + i63 := rng.Int63() + rng.Unlock() + return i63 } // Distributed pseudo-exponentially to test for various cases 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 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 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 } func RandFloat32() float32 { - return rand.Float32() + rng.Lock() + f32 := rng.Float32() + rng.Unlock() + return f32 } func RandTime() time.Time { @@ -118,11 +150,24 @@ func RandTime() time.Time { } func RandBytes(n int) []byte { - bs := make([]byte, n) - for i := 0; i < n; i++ { - bs[i] = byte(rand.Intn(256)) - } - return bs + return cRandBytes(n) +} + +// RandIntn returns, as an int, a non-negative pseudo-random number in [0, n). +// It panics if n <= 0 +func RandIntn(n int) int { + rng.Lock() + i := rng.Intn(n) + rng.Unlock() + return i +} + +// RandPerm returns a pseudo-random permutation of n integers in [0, n). +func RandPerm(n int) []int { + rng.Lock() + perm := rng.Perm(n) + rng.Unlock() + return perm } // NOTE: This relies on the os's random number generator. diff --git a/common/random_test.go b/common/random_test.go new file mode 100644 index 00000000..dd803b3f --- /dev/null +++ b/common/random_test.go @@ -0,0 +1,108 @@ +package common_test + +import ( + "io/ioutil" + "os" + "os/exec" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/tendermint/tmlibs/common" +) + +// 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) { + if os.Getenv("TENDERMINT_INTEGRATION_TESTS") == "" { + t.Skipf("Can only be run as an integration test") + } + + // The goal of this test is to invoke the + // Rand* tests externally with no repeating results, booted up. + // Any repeated results indicate that the seed is the same or that + // perhaps we are using math/rand directly. + tmpDir, err := ioutil.TempDir("", "rng-tests") + if err != nil { + t.Fatalf("Creating tempDir: %v", err) + } + defer os.RemoveAll(tmpDir) + + outpath := filepath.Join(tmpDir, "main.go") + f, err := os.Create(outpath) + if err != nil { + t.Fatalf("Setting up %q err: %v", outpath, err) + } + f.Write([]byte(integrationTestProgram)) + if err := f.Close(); err != nil { + t.Fatalf("Closing: %v", err) + } + + outputs := make(map[string][]int) + for i := 0; i < 100; i++ { + cmd := exec.Command("go", "run", outpath) + bOutput, err := cmd.CombinedOutput() + if err != nil { + t.Errorf("Run #%d: err: %v output: %s", i, err, bOutput) + continue + } + output := string(bOutput) + 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) + } +} + +const integrationTestProgram = ` +package main + +import ( + "encoding/json" + "fmt" + "math/rand" + + "github.com/tendermint/tmlibs/common" +) + +func main() { + // Set math/rand's Seed so that any direct invocations + // of math/rand will reveal themselves. + rand.Seed(1) + perm := common.RandPerm(10) + blob, _ := json.Marshal(perm) + fmt.Printf("perm: %s\n", blob) + + fmt.Printf("randInt: %d\n", common.RandInt()) + fmt.Printf("randUint: %d\n", common.RandUint()) + fmt.Printf("randIntn: %d\n", common.RandIntn(97)) + fmt.Printf("randInt31: %d\n", common.RandInt31()) + fmt.Printf("randInt32: %d\n", common.RandInt32()) + fmt.Printf("randInt63: %d\n", common.RandInt63()) + fmt.Printf("randInt64: %d\n", common.RandInt64()) + fmt.Printf("randUint32: %d\n", common.RandUint32()) + fmt.Printf("randUint64: %d\n", common.RandUint64()) + fmt.Printf("randUint16Exp: %d\n", common.RandUint16Exp()) + fmt.Printf("randUint32Exp: %d\n", common.RandUint32Exp()) + fmt.Printf("randUint64Exp: %d\n", common.RandUint64Exp()) +}` + +func TestRngConcurrencySafety(t *testing.T) { + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + + _ = common.RandUint64() + <-time.After(time.Millisecond * time.Duration(common.RandIntn(100))) + _ = common.RandPerm(3) + }() + } + wg.Wait() +} From b5f465b4ecb6ef85a6ced14728a971570ed477e0 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 15 Dec 2017 00:23:25 -0500 Subject: [PATCH 42/61] common: use names prng and mrand --- common/random.go | 61 +++++++++++++++++++++++-------------------- common/random_test.go | 21 +++++++++++++++ 2 files changed, 53 insertions(+), 29 deletions(-) diff --git a/common/random.go b/common/random.go index f0d169e0..b8304e89 100644 --- a/common/random.go +++ b/common/random.go @@ -2,7 +2,7 @@ package common import ( crand "crypto/rand" - "math/rand" + mrand "math/rand" "sync" "time" ) @@ -11,9 +11,11 @@ const ( strChars = "0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz" // 62 characters ) -var rng struct { +// pseudo random number generator. +// seeded with OS randomness (crand) +var prng struct { sync.Mutex - *rand.Rand + *mrand.Rand } func init() { @@ -23,7 +25,7 @@ func init() { seed |= uint64(b[i]) seed <<= 8 } - rng.Rand = rand.New(rand.NewSource(int64(seed))) + prng.Rand = mrand.New(mrand.NewSource(int64(seed))) } // Constructs an alphanumeric string of given length. @@ -31,7 +33,7 @@ func RandStr(length int) string { chars := []byte{} MAIN_LOOP: for { - val := rng.Int63() + val := prng.Int63() for i := 0; i < 10; i++ { v := int(val & 0x3f) // rightmost 6 bits if v >= 62 { // only 62 characters in strChars @@ -55,9 +57,9 @@ func RandUint16() uint16 { } func RandUint32() uint32 { - rng.Lock() - u32 := rng.Uint32() - rng.Unlock() + prng.Lock() + u32 := prng.Uint32() + prng.Unlock() return u32 } @@ -66,9 +68,9 @@ func RandUint64() uint64 { } func RandUint() uint { - rng.Lock() - i := rng.Int() - rng.Unlock() + prng.Lock() + i := prng.Int() + prng.Unlock() return uint(i) } @@ -85,23 +87,23 @@ func RandInt64() int64 { } func RandInt() int { - rng.Lock() - i := rng.Int() - rng.Unlock() + prng.Lock() + i := prng.Int() + prng.Unlock() return i } func RandInt31() int32 { - rng.Lock() - i31 := rng.Int31() - rng.Unlock() + prng.Lock() + i31 := prng.Int31() + prng.Unlock() return i31 } func RandInt63() int64 { - rng.Lock() - i63 := rng.Int63() - rng.Unlock() + prng.Lock() + i63 := prng.Int63() + prng.Unlock() return i63 } @@ -139,9 +141,9 @@ func RandUint64Exp() uint64 { } func RandFloat32() float32 { - rng.Lock() - f32 := rng.Float32() - rng.Unlock() + prng.Lock() + f32 := prng.Float32() + prng.Unlock() return f32 } @@ -149,6 +151,7 @@ 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. func RandBytes(n int) []byte { return cRandBytes(n) } @@ -156,17 +159,17 @@ func RandBytes(n int) []byte { // RandIntn returns, as an int, a non-negative pseudo-random number in [0, n). // It panics if n <= 0 func RandIntn(n int) int { - rng.Lock() - i := rng.Intn(n) - rng.Unlock() + prng.Lock() + i := prng.Intn(n) + prng.Unlock() return i } // RandPerm returns a pseudo-random permutation of n integers in [0, n). func RandPerm(n int) []int { - rng.Lock() - perm := rng.Perm(n) - rng.Unlock() + prng.Lock() + perm := prng.Perm(n) + prng.Unlock() return perm } diff --git a/common/random_test.go b/common/random_test.go index dd803b3f..3fe0bbc0 100644 --- a/common/random_test.go +++ b/common/random_test.go @@ -9,9 +9,30 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" "github.com/tendermint/tmlibs/common" ) +func TestRandStr(t *testing.T) { + l := 243 + s := common.RandStr(l) + assert.Equal(t, l, len(s)) +} + +func TestRandBytes(t *testing.T) { + l := 243 + b := common.RandBytes(l) + assert.Equal(t, l, len(b)) +} + +func TestRandIntn(t *testing.T) { + n := 243 + for i := 0; i < 100; i++ { + x := common.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: From cdc798882326a722040706a87ec0397e7c91d517 Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Fri, 15 Dec 2017 02:14:05 -0700 Subject: [PATCH 43/61] common: use genius simplification of tests from @ebuchman Massive test simplication for more reliable tests from @ebuchman --- common/random.go | 8 ++- common/random_test.go | 111 +++++++++++++++--------------------------- 2 files changed, 45 insertions(+), 74 deletions(-) diff --git a/common/random.go b/common/random.go index b8304e89..37b8b277 100644 --- a/common/random.go +++ b/common/random.go @@ -18,14 +18,20 @@ var prng struct { *mrand.Rand } -func init() { +func reset() { b := cRandBytes(8) var seed uint64 for i := 0; i < 8; i++ { seed |= uint64(b[i]) seed <<= 8 } + prng.Lock() prng.Rand = mrand.New(mrand.NewSource(int64(seed))) + prng.Unlock() +} + +func init() { + reset() } // Constructs an alphanumeric string of given length. diff --git a/common/random_test.go b/common/random_test.go index 3fe0bbc0..bed8e765 100644 --- a/common/random_test.go +++ b/common/random_test.go @@ -1,34 +1,34 @@ -package common_test +package common import ( - "io/ioutil" - "os" - "os/exec" - "path/filepath" + "bytes" + "encoding/json" + "fmt" + "io" + mrand "math/rand" "sync" "testing" "time" "github.com/stretchr/testify/assert" - "github.com/tendermint/tmlibs/common" ) func TestRandStr(t *testing.T) { l := 243 - s := common.RandStr(l) + s := RandStr(l) assert.Equal(t, l, len(s)) } func TestRandBytes(t *testing.T) { l := 243 - b := common.RandBytes(l) + b := RandBytes(l) assert.Equal(t, l, len(b)) } func TestRandIntn(t *testing.T) { n := 243 for i := 0; i < 100; i++ { - x := common.RandIntn(n) + x := RandIntn(n) assert.True(t, x < n) } } @@ -39,39 +39,12 @@ func TestRandIntn(t *testing.T) { // * https://github.com/tendermint/tmlibs/issues/99 // * https://github.com/tendermint/tendermint/issues/973 func TestUniqueRng(t *testing.T) { - if os.Getenv("TENDERMINT_INTEGRATION_TESTS") == "" { - t.Skipf("Can only be run as an integration test") - } - - // The goal of this test is to invoke the - // Rand* tests externally with no repeating results, booted up. - // Any repeated results indicate that the seed is the same or that - // perhaps we are using math/rand directly. - tmpDir, err := ioutil.TempDir("", "rng-tests") - if err != nil { - t.Fatalf("Creating tempDir: %v", err) - } - defer os.RemoveAll(tmpDir) - - outpath := filepath.Join(tmpDir, "main.go") - f, err := os.Create(outpath) - if err != nil { - t.Fatalf("Setting up %q err: %v", outpath, err) - } - f.Write([]byte(integrationTestProgram)) - if err := f.Close(); err != nil { - t.Fatalf("Closing: %v", err) - } - + buf := new(bytes.Buffer) outputs := make(map[string][]int) for i := 0; i < 100; i++ { - cmd := exec.Command("go", "run", outpath) - bOutput, err := cmd.CombinedOutput() - if err != nil { - t.Errorf("Run #%d: err: %v output: %s", i, err, bOutput) - continue - } - output := string(bOutput) + 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) @@ -80,38 +53,30 @@ func TestUniqueRng(t *testing.T) { } } -const integrationTestProgram = ` -package main +func testThemAll(out io.Writer) { + // Reset the internal PRNG + reset() -import ( - "encoding/json" - "fmt" - "math/rand" + // 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) - "github.com/tendermint/tmlibs/common" -) - -func main() { - // Set math/rand's Seed so that any direct invocations - // of math/rand will reveal themselves. - rand.Seed(1) - perm := common.RandPerm(10) - blob, _ := json.Marshal(perm) - fmt.Printf("perm: %s\n", blob) - - fmt.Printf("randInt: %d\n", common.RandInt()) - fmt.Printf("randUint: %d\n", common.RandUint()) - fmt.Printf("randIntn: %d\n", common.RandIntn(97)) - fmt.Printf("randInt31: %d\n", common.RandInt31()) - fmt.Printf("randInt32: %d\n", common.RandInt32()) - fmt.Printf("randInt63: %d\n", common.RandInt63()) - fmt.Printf("randInt64: %d\n", common.RandInt64()) - fmt.Printf("randUint32: %d\n", common.RandUint32()) - fmt.Printf("randUint64: %d\n", common.RandUint64()) - fmt.Printf("randUint16Exp: %d\n", common.RandUint16Exp()) - fmt.Printf("randUint32Exp: %d\n", common.RandUint32Exp()) - fmt.Printf("randUint64Exp: %d\n", common.RandUint64Exp()) -}` + 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 @@ -120,9 +85,9 @@ func TestRngConcurrencySafety(t *testing.T) { go func() { defer wg.Done() - _ = common.RandUint64() - <-time.After(time.Millisecond * time.Duration(common.RandIntn(100))) - _ = common.RandPerm(3) + _ = RandUint64() + <-time.After(time.Millisecond * time.Duration(RandIntn(100))) + _ = RandPerm(3) }() } wg.Wait() From 8638961f02833def91f743cbccaa2cecdccffa74 Mon Sep 17 00:00:00 2001 From: Emmanuel Odeke Date: Fri, 15 Dec 2017 22:40:12 -0700 Subject: [PATCH 44/61] common: Rand* warnings about cryptographic unsafety Lesson articulated by @jaekwon on why we need 80 bits of entropy at least before we can think of cryptographic safety. math/rand's seed is a max of 64 bits so can never be cryptographically secure. Also added some benchmarks for RandBytes --- common/random.go | 29 +++++++++++++++++++++++++++-- common/random_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/common/random.go b/common/random.go index 37b8b277..9df55ff8 100644 --- a/common/random.go +++ b/common/random.go @@ -35,6 +35,7 @@ func init() { } // Constructs an alphanumeric string of given length. +// It is not safe for cryptographic usage. func RandStr(length int) string { chars := []byte{} MAIN_LOOP: @@ -58,10 +59,12 @@ MAIN_LOOP: return string(chars) } +// It is not safe for cryptographic usage. func RandUint16() uint16 { return uint16(RandUint32() & (1<<16 - 1)) } +// It is not safe for cryptographic usage. func RandUint32() uint32 { prng.Lock() u32 := prng.Uint32() @@ -69,10 +72,12 @@ func RandUint32() uint32 { return u32 } +// It is not safe for cryptographic usage. func RandUint64() uint64 { return uint64(RandUint32())<<32 + uint64(RandUint32()) } +// It is not safe for cryptographic usage. func RandUint() uint { prng.Lock() i := prng.Int() @@ -80,18 +85,22 @@ func RandUint() uint { return uint(i) } +// It is not safe for cryptographic usage. func RandInt16() int16 { return int16(RandUint32() & (1<<16 - 1)) } +// It is not safe for cryptographic usage. func RandInt32() int32 { return int32(RandUint32()) } +// It is not safe for cryptographic usage. func RandInt64() int64 { return int64(RandUint64()) } +// It is not safe for cryptographic usage. func RandInt() int { prng.Lock() i := prng.Int() @@ -99,6 +108,7 @@ func RandInt() int { return i } +// It is not safe for cryptographic usage. func RandInt31() int32 { prng.Lock() i31 := prng.Int31() @@ -106,6 +116,7 @@ func RandInt31() int32 { return i31 } +// It is not safe for cryptographic usage. func RandInt63() int64 { prng.Lock() i63 := prng.Int63() @@ -114,6 +125,7 @@ func RandInt63() int64 { } // Distributed pseudo-exponentially to test for various cases +// It is not safe for cryptographic usage. func RandUint16Exp() uint16 { bits := RandUint32() % 16 if bits == 0 { @@ -125,6 +137,7 @@ func RandUint16Exp() uint16 { } // Distributed pseudo-exponentially to test for various cases +// It is not safe for cryptographic usage. func RandUint32Exp() uint32 { bits := RandUint32() % 32 if bits == 0 { @@ -136,6 +149,7 @@ func RandUint32Exp() uint32 { } // Distributed pseudo-exponentially to test for various cases +// It is not safe for cryptographic usage. func RandUint64Exp() uint64 { bits := RandUint32() % 64 if bits == 0 { @@ -146,6 +160,7 @@ func RandUint64Exp() uint64 { return n } +// It is not safe for cryptographic usage. func RandFloat32() float32 { prng.Lock() f32 := prng.Float32() @@ -153,17 +168,26 @@ func RandFloat32() float32 { 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 { - return cRandBytes(n) + // 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 < 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 panics if n <= 0. +// It is not safe for cryptographic usage. func RandIntn(n int) int { prng.Lock() i := prng.Intn(n) @@ -172,6 +196,7 @@ func RandIntn(n int) int { } // 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) diff --git a/common/random_test.go b/common/random_test.go index bed8e765..216f2f8b 100644 --- a/common/random_test.go +++ b/common/random_test.go @@ -92,3 +92,29 @@ func TestRngConcurrencySafety(t *testing.T) { } 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() +} From 70e30f74e60b2710c3c270178b0be2c4c7319722 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 19 Dec 2017 16:16:16 -0600 Subject: [PATCH 45/61] Revert "Refactor repeat timer" --- common/repeat_timer.go | 116 +++++++++++++++------------------- common/repeat_timer_test.go | 12 ++-- common/throttle_timer.go | 24 ++++--- common/throttle_timer_test.go | 2 - 4 files changed, 67 insertions(+), 87 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 23faf74a..d7d9154d 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -1,6 +1,7 @@ package common import ( + "sync" "time" ) @@ -10,93 +11,76 @@ It's good for keeping connections alive. A RepeatTimer must be Stop()'d or it will keep a goroutine alive. */ type RepeatTimer struct { - Name string - Ch <-chan time.Time - output chan<- time.Time - input chan repeatCommand + Ch chan time.Time - dur time.Duration - ticker *time.Ticker - stopped bool + mtx sync.Mutex + name string + ticker *time.Ticker + quit chan struct{} + wg *sync.WaitGroup + dur time.Duration } -type repeatCommand int8 - -const ( - Reset repeatCommand = iota - RQuit -) - func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer { - c := make(chan time.Time) var t = &RepeatTimer{ - Name: name, - Ch: c, - output: c, - input: make(chan repeatCommand), - - dur: dur, + Ch: make(chan time.Time), ticker: time.NewTicker(dur), + quit: make(chan struct{}), + wg: new(sync.WaitGroup), + name: name, + dur: dur, } - go t.run() + t.wg.Add(1) + go t.fireRoutine(t.ticker) return t } +func (t *RepeatTimer) fireRoutine(ticker *time.Ticker) { + 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() + return + } + } +} + // Wait the duration again before firing. func (t *RepeatTimer) Reset() { - t.input <- 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) } // For ease of .Stop()'ing services before .Start()'ing them, // we ignore .Stop()'s on nil RepeatTimers. func (t *RepeatTimer) Stop() bool { - // use t.stopped to gracefully handle many Stop() without blocking - if t == nil || t.stopped { + if t == nil { return false } - t.input <- RQuit - t.stopped = true - return true -} + t.mtx.Lock() // Lock + defer t.mtx.Unlock() -func (t *RepeatTimer) run() { - done := false - for !done { + exists := t.ticker != nil + if exists { + t.ticker.Stop() // does not close the channel select { - case cmd := <-t.input: - // stop goroutine if the input says so - // don't close channels, as closed channels mess up select reads - done = t.processInput(cmd) - case tick := <-t.ticker.C: - t.send(tick) + 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 } -} - -// send performs blocking send on t.Ch -func (t *RepeatTimer) send(tick time.Time) { - // XXX: possibly it is better to not block: - // https://golang.org/src/time/sleep.go#L132 - // select { - // case t.output <- tick: - // default: - // } - t.output <- tick -} - -// all modifications of the internal state of ThrottleTimer -// happen in this method. It is only called from the run goroutine -// so we avoid any race conditions -func (t *RepeatTimer) processInput(cmd repeatCommand) (shutdown bool) { - switch cmd { - case Reset: - t.ticker.Stop() - t.ticker = time.NewTicker(t.dur) - case RQuit: - t.ticker.Stop() - shutdown = true - default: - panic("unknown command!") - } - return shutdown + return exists } diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index db53aa61..87f34b95 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -10,7 +10,7 @@ import ( ) type rCounter struct { - input <-chan time.Time + input chan time.Time mtx sync.Mutex count int } @@ -39,11 +39,11 @@ func (c *rCounter) Read() { func TestRepeat(test *testing.T) { assert := asrt.New(test) - dur := time.Duration(100) * time.Millisecond + dur := time.Duration(50) * time.Millisecond short := time.Duration(20) * time.Millisecond // delay waits for cnt durations, an a little extra delay := func(cnt int) time.Duration { - return time.Duration(cnt)*dur + time.Duration(10)*time.Millisecond + return time.Duration(cnt)*dur + time.Duration(5)*time.Millisecond } t := NewRepeatTimer("bar", dur) @@ -70,9 +70,9 @@ func TestRepeat(test *testing.T) { // after a stop, nothing more is sent stopped := t.Stop() assert.True(stopped) - time.Sleep(delay(2)) + time.Sleep(delay(7)) assert.Equal(6, c.Count()) - // extra calls to stop don't block - t.Stop() + // close channel to stop counter + close(t.Ch) } diff --git a/common/throttle_timer.go b/common/throttle_timer.go index a5bd6ded..ab2ad2e6 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -13,21 +13,20 @@ at most once every "dur". type ThrottleTimer struct { Name string Ch <-chan struct{} - input chan throttleCommand + input chan command output chan<- struct{} dur time.Duration - timer *time.Timer - isSet bool - stopped bool + timer *time.Timer + isSet bool } -type throttleCommand int8 +type command int32 const ( - Set throttleCommand = iota + Set command = iota Unset - TQuit + Quit ) // NewThrottleTimer creates a new ThrottleTimer. @@ -37,7 +36,7 @@ func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { Name: name, Ch: c, dur: dur, - input: make(chan throttleCommand), + input: make(chan command), output: c, timer: time.NewTimer(dur), } @@ -75,14 +74,14 @@ func (t *ThrottleTimer) trySend() { // all modifications of the internal state of ThrottleTimer // happen in this method. It is only called from the run goroutine // so we avoid any race conditions -func (t *ThrottleTimer) processInput(cmd throttleCommand) (shutdown bool) { +func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { switch cmd { case Set: if !t.isSet { t.isSet = true t.timer.Reset(t.dur) } - case TQuit: + case Quit: shutdown = true fallthrough case Unset: @@ -120,10 +119,9 @@ func (t *ThrottleTimer) Unset() { // For ease of stopping services before starting them, we ignore Stop on nil // ThrottleTimers. func (t *ThrottleTimer) Stop() bool { - if t == nil || t.stopped { + if t == nil { return false } - t.input <- TQuit - t.stopped = true + t.input <- Quit return true } diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index 94ec1b43..a1b6606f 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -95,6 +95,4 @@ func TestThrottle(test *testing.T) { stopped := t.Stop() assert.True(stopped) - // extra calls to stop don't block - t.Stop() } From e17e8e425f43890b207e5e316f5190d278e849c3 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 19 Dec 2017 16:23:20 -0600 Subject: [PATCH 46/61] Revert "Refactor throttle timer" --- common/repeat_timer_test.go | 2 +- common/throttle_timer.go | 120 ++++++++++------------------------ common/throttle_timer_test.go | 24 +------ 3 files changed, 37 insertions(+), 109 deletions(-) diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 87f34b95..9f03f41d 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -43,7 +43,7 @@ func TestRepeat(test *testing.T) { short := time.Duration(20) * time.Millisecond // delay waits for cnt durations, an a little extra delay := func(cnt int) time.Duration { - return time.Duration(cnt)*dur + time.Duration(5)*time.Millisecond + return time.Duration(cnt)*dur + time.Millisecond } t := NewRepeatTimer("bar", dur) diff --git a/common/throttle_timer.go b/common/throttle_timer.go index ab2ad2e6..38ef4e9a 100644 --- a/common/throttle_timer.go +++ b/common/throttle_timer.go @@ -1,6 +1,7 @@ package common import ( + "sync" "time" ) @@ -11,117 +12,64 @@ If a long continuous burst of .Set() calls happens, ThrottleTimer fires at most once every "dur". */ type ThrottleTimer struct { - Name string - Ch <-chan struct{} - input chan command - output chan<- struct{} - dur time.Duration + Name string + Ch chan struct{} + quit chan struct{} + dur time.Duration + mtx sync.Mutex timer *time.Timer isSet bool } -type command int32 - -const ( - Set command = iota - Unset - Quit -) - -// NewThrottleTimer creates a new ThrottleTimer. func NewThrottleTimer(name string, dur time.Duration) *ThrottleTimer { - c := make(chan struct{}) - var t = &ThrottleTimer{ - Name: name, - Ch: c, - dur: dur, - input: make(chan command), - output: c, - timer: time.NewTimer(dur), - } + var ch = make(chan struct{}) + var quit = make(chan struct{}) + var t = &ThrottleTimer{Name: name, Ch: ch, dur: dur, quit: quit} + t.mtx.Lock() + t.timer = time.AfterFunc(dur, t.fireRoutine) + t.mtx.Unlock() t.timer.Stop() - go t.run() return t } -func (t *ThrottleTimer) run() { - for { - select { - case cmd := <-t.input: - // stop goroutine if the input says so - // don't close channels, as closed channels mess up select reads - if t.processInput(cmd) { - return - } - case <-t.timer.C: - t.trySend() - } - } -} - -// trySend performs non-blocking send on t.Ch -func (t *ThrottleTimer) trySend() { +func (t *ThrottleTimer) fireRoutine() { + t.mtx.Lock() + defer t.mtx.Unlock() select { - case t.output <- struct{}{}: + case t.Ch <- struct{}{}: t.isSet = false + case <-t.quit: + // do nothing default: - // if we just want to drop, replace this with t.isSet = false t.timer.Reset(t.dur) } } -// all modifications of the internal state of ThrottleTimer -// happen in this method. It is only called from the run goroutine -// so we avoid any race conditions -func (t *ThrottleTimer) processInput(cmd command) (shutdown bool) { - switch cmd { - case Set: - if !t.isSet { - t.isSet = true - t.timer.Reset(t.dur) - } - case Quit: - shutdown = true - fallthrough - case Unset: - if t.isSet { - t.isSet = false - t.timer.Stop() - } - default: - panic("unknown command!") - } - return shutdown -} - func (t *ThrottleTimer) Set() { - t.input <- Set + t.mtx.Lock() + defer t.mtx.Unlock() + if !t.isSet { + t.isSet = true + t.timer.Reset(t.dur) + } } func (t *ThrottleTimer) Unset() { - t.input <- Unset + t.mtx.Lock() + defer t.mtx.Unlock() + t.isSet = false + t.timer.Stop() } -// Stop prevents the ThrottleTimer from firing. It always returns true. Stop does not -// close the channel, to prevent a read from the channel succeeding -// incorrectly. -// -// To prevent a timer created with NewThrottleTimer from firing after a call to -// Stop, check the return value and drain the channel. -// -// For example, assuming the program has not received from t.C already: -// -// if !t.Stop() { -// <-t.C -// } -// -// For ease of stopping services before starting them, we ignore Stop on nil -// ThrottleTimers. +// For ease of .Stop()'ing services before .Start()'ing them, +// we ignore .Stop()'s on nil ThrottleTimers func (t *ThrottleTimer) Stop() bool { if t == nil { return false } - t.input <- Quit - return true + close(t.quit) + t.mtx.Lock() + defer t.mtx.Unlock() + return t.timer.Stop() } diff --git a/common/throttle_timer_test.go b/common/throttle_timer_test.go index a1b6606f..00f5abde 100644 --- a/common/throttle_timer_test.go +++ b/common/throttle_timer_test.go @@ -10,7 +10,7 @@ import ( ) type thCounter struct { - input <-chan struct{} + input chan struct{} mtx sync.Mutex count int } @@ -31,9 +31,6 @@ func (c *thCounter) Count() int { // Read should run in a go-routine and // updates count by one every time a packet comes in func (c *thCounter) Read() { - // note, since this channel never closes, this will never end - // if thCounter was used in anything beyond trivial test cases. - // it would have to be smarter. for range c.input { c.Increment() } @@ -44,7 +41,6 @@ func TestThrottle(test *testing.T) { ms := 50 delay := time.Duration(ms) * time.Millisecond - shortwait := time.Duration(ms/2) * time.Millisecond longwait := time.Duration(2) * delay t := NewThrottleTimer("foo", delay) @@ -69,21 +65,6 @@ func TestThrottle(test *testing.T) { time.Sleep(longwait) assert.Equal(2, c.Count()) - // keep cancelling before it is ready - for i := 0; i < 10; i++ { - t.Set() - time.Sleep(shortwait) - t.Unset() - } - time.Sleep(longwait) - assert.Equal(2, c.Count()) - - // a few unsets do nothing... - for i := 0; i < 5; i++ { - t.Unset() - } - 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++ { @@ -93,6 +74,5 @@ func TestThrottle(test *testing.T) { time.Sleep(longwait) assert.Equal(5, c.Count()) - stopped := t.Stop() - assert.True(stopped) + close(t.Ch) } From a25ed5ba1b0124f82f77b722cf3225cf4b3f18f5 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 21 Dec 2017 10:02:25 -0500 Subject: [PATCH 47/61] cmn: fix race condition in prng --- common/random.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/random.go b/common/random.go index 9df55ff8..ca71b614 100644 --- a/common/random.go +++ b/common/random.go @@ -40,7 +40,7 @@ func RandStr(length int) string { chars := []byte{} MAIN_LOOP: for { - val := prng.Int63() + val := RandInt63() for i := 0; i < 10; i++ { v := int(val & 0x3f) // rightmost 6 bits if v >= 62 { // only 62 characters in strChars From b0b740210c60b7fc789382ff3a709426eb71903d Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 21 Dec 2017 11:15:17 -0500 Subject: [PATCH 48/61] cmn: fix repeate timer test with manual ticker --- common/repeat_timer.go | 86 +++++++++++++++++++++++++++++++++---- common/repeat_timer_test.go | 81 ++++++++++++++++------------------ 2 files changed, 114 insertions(+), 53 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index d7d9154d..1500e95d 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -5,6 +5,72 @@ import ( "time" ) +// Ticker is a basic ticker interface. +type Ticker interface { + Chan() <-chan time.Time + Stop() + Reset() +} + +// DefaultTicker wraps the stdlibs Ticker implementation. +type DefaultTicker struct { + t *time.Ticker + dur time.Duration +} + +// NewDefaultTicker returns a new DefaultTicker +func NewDefaultTicker(dur time.Duration) *DefaultTicker { + return &DefaultTicker{ + time.NewTicker(dur), + dur, + } +} + +// Implements Ticker +func (t *DefaultTicker) Chan() <-chan time.Time { + return t.t.C +} + +// Implements Ticker +func (t *DefaultTicker) Stop() { + t.t.Stop() + t.t = nil +} + +// Implements Ticker +func (t *DefaultTicker) Reset() { + t.t = time.NewTicker(t.dur) +} + +// ManualTicker wraps a channel that can be manually sent on +type ManualTicker struct { + ch chan time.Time +} + +// NewManualTicker returns a new ManualTicker +func NewManualTicker(ch chan time.Time) *ManualTicker { + return &ManualTicker{ + ch: ch, + } +} + +// Implements Ticker +func (t *ManualTicker) Chan() <-chan time.Time { + return t.ch +} + +// Implements Ticker +func (t *ManualTicker) Stop() { + // noop +} + +// Implements Ticker +func (t *ManualTicker) Reset() { + // noop +} + +//--------------------------------------------------------------------- + /* RepeatTimer repeatedly sends a struct{}{} to .Ch after each "dur" period. It's good for keeping connections alive. @@ -15,30 +81,35 @@ type RepeatTimer struct { mtx sync.Mutex name string - ticker *time.Ticker + ticker Ticker quit chan struct{} wg *sync.WaitGroup - dur time.Duration } +// NewRepeatTimer returns a RepeatTimer with the DefaultTicker. func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer { + ticker := NewDefaultTicker(dur) + return NewRepeatTimerWithTicker(name, ticker) +} + +// NewRepeatTimerWithTicker returns a RepeatTimer with the given ticker. +func NewRepeatTimerWithTicker(name string, ticker Ticker) *RepeatTimer { var t = &RepeatTimer{ Ch: make(chan time.Time), - ticker: time.NewTicker(dur), + ticker: ticker, quit: make(chan struct{}), wg: new(sync.WaitGroup), name: name, - dur: dur, } t.wg.Add(1) go t.fireRoutine(t.ticker) return t } -func (t *RepeatTimer) fireRoutine(ticker *time.Ticker) { +func (t *RepeatTimer) fireRoutine(ticker Ticker) { for { select { - case t_ := <-ticker.C: + case t_ := <-ticker.Chan(): t.Ch <- t_ case <-t.quit: // needed so we know when we can reset t.quit @@ -55,7 +126,7 @@ func (t *RepeatTimer) Reset() { t.mtx.Lock() // Lock defer t.mtx.Unlock() - t.ticker = time.NewTicker(t.dur) + t.ticker.Reset() t.quit = make(chan struct{}) t.wg.Add(1) go t.fireRoutine(t.ticker) @@ -80,7 +151,6 @@ func (t *RepeatTimer) Stop() bool { } close(t.quit) t.wg.Wait() // must wait for quit to close else we race Reset - t.ticker = nil } return exists } diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 9f03f41d..98d991e9 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -1,7 +1,6 @@ package common import ( - "sync" "testing" "time" @@ -9,69 +8,61 @@ import ( asrt "github.com/stretchr/testify/assert" ) -type rCounter struct { - input chan time.Time - mtx sync.Mutex - count int -} - -func (c *rCounter) Increment() { - c.mtx.Lock() - c.count++ - c.mtx.Unlock() -} - -func (c *rCounter) 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 *rCounter) Read() { - for range c.input { - c.Increment() - } -} - +// NOTE: this only tests with the ManualTicker. +// How do you test a real-clock ticker properly? func TestRepeat(test *testing.T) { assert := asrt.New(test) - dur := time.Duration(50) * time.Millisecond - short := time.Duration(20) * time.Millisecond - // delay waits for cnt durations, an a little extra - delay := func(cnt int) time.Duration { - return time.Duration(cnt)*dur + time.Millisecond + ch := make(chan time.Time, 100) + // tick fires cnt times on ch + tick := func(cnt int) { + for i := 0; i < cnt; i++ { + ch <- time.Now() + } } - t := NewRepeatTimer("bar", dur) + tock := func(test *testing.T, t *RepeatTimer, cnt int) { + for i := 0; i < cnt; i++ { + after := time.After(time.Second * 2) + select { + case <-t.Ch: + case <-after: + test.Fatal("expected ticker to fire") + } + } + done := true + select { + case <-t.Ch: + done = false + default: + } + assert.True(done) + } + + ticker := NewManualTicker(ch) + t := NewRepeatTimerWithTicker("bar", ticker) // start at 0 - c := &rCounter{input: t.Ch} - go c.Read() - assert.Equal(0, c.Count()) + tock(test, t, 0) // wait for 4 periods - time.Sleep(delay(4)) - assert.Equal(4, c.Count()) + tick(4) + tock(test, t, 4) // keep reseting leads to no firing for i := 0; i < 20; i++ { - time.Sleep(short) + time.Sleep(time.Millisecond) t.Reset() } - assert.Equal(4, c.Count()) + tock(test, t, 0) // after this, it still works normal - time.Sleep(delay(2)) - assert.Equal(6, c.Count()) + tick(2) + tock(test, t, 2) // after a stop, nothing more is sent stopped := t.Stop() assert.True(stopped) - time.Sleep(delay(7)) - assert.Equal(6, c.Count()) + tock(test, t, 0) // close channel to stop counter close(t.Ch) From e2d7f1aa41dde5f29057dd08e64371a574b84c86 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Thu, 21 Dec 2017 14:21:15 -0500 Subject: [PATCH 49/61] cmn: fix race --- common/repeat_timer.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 1500e95d..0bc4d87b 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -102,14 +102,14 @@ func NewRepeatTimerWithTicker(name string, ticker Ticker) *RepeatTimer { name: name, } t.wg.Add(1) - go t.fireRoutine(t.ticker) + go t.fireRoutine(t.ticker.Chan()) return t } -func (t *RepeatTimer) fireRoutine(ticker Ticker) { +func (t *RepeatTimer) fireRoutine(ch <-chan time.Time) { for { select { - case t_ := <-ticker.Chan(): + case t_ := <-ch: t.Ch <- t_ case <-t.quit: // needed so we know when we can reset t.quit @@ -129,7 +129,7 @@ func (t *RepeatTimer) Reset() { t.ticker.Reset() t.quit = make(chan struct{}) t.wg.Add(1) - go t.fireRoutine(t.ticker) + go t.fireRoutine(t.ticker.Chan()) } // For ease of .Stop()'ing services before .Start()'ing them, From 2fd8f35b74e80382e276393b6edaa4464642a9df Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Mon, 25 Dec 2017 21:12:14 -0800 Subject: [PATCH 50/61] Fix #112 by using RWMutex per element --- clist/clist.go | 222 +++++++++++++++++++++++++++++-------------------- 1 file changed, 130 insertions(+), 92 deletions(-) diff --git a/clist/clist.go b/clist/clist.go index 5295dd99..e8cf6b93 100644 --- a/clist/clist.go +++ b/clist/clist.go @@ -11,36 +11,38 @@ to ensure garbage collection of removed elements. import ( "sync" - "sync/atomic" - "unsafe" ) // CElement is an element of a linked-list // Traversal from a CElement are goroutine-safe. type CElement struct { - prev unsafe.Pointer + mtx sync.RWMutex + prev *CElement prevWg *sync.WaitGroup - next unsafe.Pointer + next *CElement nextWg *sync.WaitGroup - removed uint32 - Value interface{} + removed bool + + Value interface{} // immutable } // Blocking implementation of Next(). // May return nil iff CElement was tail and got removed. func (e *CElement) NextWait() *CElement { for { - e.nextWg.Wait() - next := e.Next() - if next == nil { - if e.Removed() { - return nil - } else { - continue - } - } else { + e.mtx.RLock() + next := e.next + nextWg := e.nextWg + removed := e.removed + e.mtx.RUnlock() + + if next != nil || removed { return next } + + nextWg.Wait() + // e.next doesn't necessarily exist here. + // That's why we need to continue a for-loop. } } @@ -48,82 +50,113 @@ func (e *CElement) NextWait() *CElement { // May return nil iff CElement was head and got removed. func (e *CElement) PrevWait() *CElement { for { - e.prevWg.Wait() - prev := e.Prev() - if prev == nil { - if e.Removed() { - return nil - } else { - continue - } - } else { + e.mtx.RLock() + prev := e.prev + prevWg := e.prevWg + removed := e.removed + e.mtx.RUnlock() + + if prev != nil || removed { return prev } + + prevWg.Wait() } } // Nonblocking, may return nil if at the end. func (e *CElement) Next() *CElement { - return (*CElement)(atomic.LoadPointer(&e.next)) + e.mtx.RLock() + defer e.mtx.RUnlock() + + return e.next } // Nonblocking, may return nil if at the end. func (e *CElement) Prev() *CElement { - return (*CElement)(atomic.LoadPointer(&e.prev)) + e.mtx.RLock() + defer e.mtx.RUnlock() + + return e.prev } func (e *CElement) Removed() bool { - return atomic.LoadUint32(&(e.removed)) > 0 + e.mtx.RLock() + defer e.mtx.RUnlock() + + return e.removed } func (e *CElement) DetachNext() { if !e.Removed() { panic("DetachNext() must be called after Remove(e)") } - atomic.StorePointer(&e.next, nil) + e.mtx.Lock() + defer e.mtx.Unlock() + + e.next = nil } func (e *CElement) DetachPrev() { if !e.Removed() { panic("DetachPrev() must be called after Remove(e)") } - atomic.StorePointer(&e.prev, nil) + e.mtx.Lock() + defer e.mtx.Unlock() + + e.prev = nil } -func (e *CElement) setNextAtomic(next *CElement) { - for { - oldNext := atomic.LoadPointer(&e.next) - if !atomic.CompareAndSwapPointer(&(e.next), oldNext, unsafe.Pointer(next)) { - continue - } - if next == nil && oldNext != nil { // We for-loop in NextWait() so race is ok - e.nextWg.Add(1) - } - if next != nil && oldNext == nil { - e.nextWg.Done() - } - return +// NOTE: This function needs to be safe for +// concurrent goroutines waiting on nextWg. +func (e *CElement) SetNext(newNext *CElement) { + e.mtx.Lock() + defer e.mtx.Unlock() + + oldNext := e.next + e.next = newNext + if oldNext != nil && newNext == nil { + // See https://golang.org/pkg/sync/: + // + // If a WaitGroup is reused to wait for several independent sets of + // events, new Add calls must happen after all previous Wait calls have + // returned. + e.nextWg = waitGroup1() // WaitGroups are difficult to re-use. + } + if oldNext == nil && newNext != nil { + e.nextWg.Done() } } -func (e *CElement) setPrevAtomic(prev *CElement) { - for { - oldPrev := atomic.LoadPointer(&e.prev) - if !atomic.CompareAndSwapPointer(&(e.prev), oldPrev, unsafe.Pointer(prev)) { - continue - } - if prev == nil && oldPrev != nil { // We for-loop in PrevWait() so race is ok - e.prevWg.Add(1) - } - if prev != nil && oldPrev == nil { - e.prevWg.Done() - } - return +// NOTE: This function needs to be safe for +// concurrent goroutines waiting on prevWg +func (e *CElement) SetPrev(newPrev *CElement) { + e.mtx.Lock() + defer e.mtx.Unlock() + + oldPrev := e.prev + e.prev = newPrev + if oldPrev != nil && newPrev == nil { + e.prevWg = waitGroup1() // WaitGroups are difficult to re-use. + } + if oldPrev == nil && newPrev != nil { + e.prevWg.Done() } } -func (e *CElement) setRemovedAtomic() { - atomic.StoreUint32(&(e.removed), 1) +func (e *CElement) SetRemoved() { + e.mtx.Lock() + defer e.mtx.Unlock() + + e.removed = true + + // This wakes up anyone waiting in either direction. + if e.prev == nil { + e.prevWg.Done() + } + if e.next == nil { + e.nextWg.Done() + } } //-------------------------------------------------------------------------------- @@ -132,7 +165,7 @@ func (e *CElement) setRemovedAtomic() { // The zero value for CList is an empty list ready to use. // Operations are goroutine-safe. type CList struct { - mtx sync.Mutex + mtx sync.RWMutex wg *sync.WaitGroup head *CElement // first element tail *CElement // last element @@ -142,6 +175,7 @@ type CList struct { func (l *CList) Init() *CList { l.mtx.Lock() defer l.mtx.Unlock() + l.wg = waitGroup1() l.head = nil l.tail = nil @@ -152,48 +186,55 @@ func (l *CList) Init() *CList { func New() *CList { return new(CList).Init() } func (l *CList) Len() int { - l.mtx.Lock() - defer l.mtx.Unlock() + l.mtx.RLock() + defer l.mtx.RUnlock() + return l.len } func (l *CList) Front() *CElement { - l.mtx.Lock() - defer l.mtx.Unlock() + l.mtx.RLock() + defer l.mtx.RUnlock() + return l.head } func (l *CList) FrontWait() *CElement { for { - l.mtx.Lock() + l.mtx.RLock() head := l.head wg := l.wg - l.mtx.Unlock() - if head == nil { - wg.Wait() - } else { + l.mtx.RUnlock() + + if head != nil { return head } + wg.Wait() + // l.head doesn't necessarily exist here. + // That's why we need to continue a for-loop. } } func (l *CList) Back() *CElement { - l.mtx.Lock() - defer l.mtx.Unlock() + l.mtx.RLock() + defer l.mtx.RUnlock() + return l.tail } func (l *CList) BackWait() *CElement { for { - l.mtx.Lock() + l.mtx.RLock() tail := l.tail wg := l.wg - l.mtx.Unlock() - if tail == nil { - wg.Wait() - } else { + l.mtx.RUnlock() + + if tail != nil { return tail } + wg.Wait() + // l.tail doesn't necessarily exist here. + // That's why we need to continue a for-loop. } } @@ -203,11 +244,12 @@ func (l *CList) PushBack(v interface{}) *CElement { // Construct a new element e := &CElement{ - prev: nil, - prevWg: waitGroup1(), - next: nil, - nextWg: waitGroup1(), - Value: v, + prev: nil, + prevWg: waitGroup1(), + next: nil, + nextWg: waitGroup1(), + removed: false, + Value: v, } // Release waiters on FrontWait/BackWait maybe @@ -221,9 +263,9 @@ func (l *CList) PushBack(v interface{}) *CElement { l.head = e l.tail = e } else { - l.tail.setNextAtomic(e) - e.setPrevAtomic(l.tail) - l.tail = e + e.SetPrev(l.tail) // We must init e first. + l.tail.SetNext(e) // This will make e accessible. + l.tail = e // Update the list. } return e @@ -250,30 +292,26 @@ func (l *CList) Remove(e *CElement) interface{} { // If we're removing the only item, make CList FrontWait/BackWait wait. if l.len == 1 { - l.wg.Add(1) + l.wg = waitGroup1() // WaitGroups are difficult to re-use. } + + // Update l.len l.len -= 1 // Connect next/prev and set head/tail if prev == nil { l.head = next } else { - prev.setNextAtomic(next) + prev.SetNext(next) } if next == nil { l.tail = prev } else { - next.setPrevAtomic(prev) + next.SetPrev(prev) } // Set .Done() on e, otherwise waiters will wait forever. - e.setRemovedAtomic() - if prev == nil { - e.prevWg.Done() - } - if next == nil { - e.nextWg.Done() - } + e.SetRemoved() return e.Value } From 0f8ebd024db7f32ca0d94e7f3d13049ffcb70c09 Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Mon, 25 Dec 2017 22:28:15 -0800 Subject: [PATCH 51/61] Update clist.go Add more justification of synchrony primitives in documentation. --- clist/clist.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/clist/clist.go b/clist/clist.go index e8cf6b93..02e31a50 100644 --- a/clist/clist.go +++ b/clist/clist.go @@ -1,20 +1,40 @@ package clist /* + The purpose of CList is to provide a goroutine-safe linked-list. This list can be traversed concurrently by any number of goroutines. However, removed CElements cannot be added back. NOTE: Not all methods of container/list are (yet) implemented. NOTE: Removed elements need to DetachPrev or DetachNext consistently to ensure garbage collection of removed elements. + */ import ( "sync" ) -// CElement is an element of a linked-list -// Traversal from a CElement are goroutine-safe. +/* + +CElement is an element of a linked-list +Traversal from a CElement are goroutine-safe. + +We can't avoid using WaitGroups or for-loops given the documentation +spec without re-implementing the primitives that already exist in +golang/sync. Notice that WaitGroup allows many go-routines to be +simultaneously released, which is what we want. Mutex doesn't do +this. RWMutex does this, but it's clumsy to use in the way that a +WaitGroup would be used -- and we'd end up having two RWMutex's for +prev/next each, which is doubly confusing. + +sync.Cond would be sort-of useful, but we don't need a write-lock in +the for-loop. Use sync.Cond when you need serial access to the +"condition". In our case our condition is if `next != nil || removed`, +and there's no reason to serialize that condition for goroutines +waiting on NextWait() (since it's just a read operation). + +*/ type CElement struct { mtx sync.RWMutex prev *CElement From e47ce81422e436459dabf803676d3a3d6924699b Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Thu, 28 Dec 2017 03:02:23 -0800 Subject: [PATCH 52/61] Comment fixes from Emmanuel --- clist/clist.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/clist/clist.go b/clist/clist.go index 02e31a50..a52920f8 100644 --- a/clist/clist.go +++ b/clist/clist.go @@ -18,7 +18,7 @@ import ( /* CElement is an element of a linked-list -Traversal from a CElement are goroutine-safe. +Traversal from a CElement is goroutine-safe. We can't avoid using WaitGroups or for-loops given the documentation spec without re-implementing the primitives that already exist in @@ -220,6 +220,7 @@ func (l *CList) Front() *CElement { } func (l *CList) FrontWait() *CElement { + // Loop until the head is non-nil else wait and try again for { l.mtx.RLock() head := l.head @@ -230,8 +231,7 @@ func (l *CList) FrontWait() *CElement { return head } wg.Wait() - // l.head doesn't necessarily exist here. - // That's why we need to continue a for-loop. + // NOTE: If you think l.head exists here, think harder. } } From 6b5d08f7daf180036d338d7d7d729861bb58eae5 Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Sat, 23 Dec 2017 04:18:50 -0800 Subject: [PATCH 53/61] RepeatTimer fix --- common/repeat_timer.go | 262 +++++++++++++++++++++++------------- common/repeat_timer_test.go | 86 +++++++----- 2 files changed, 220 insertions(+), 128 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 0bc4d87b..2947a916 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -5,152 +5,224 @@ import ( "time" ) +// Used by RepeatTimer the first time, +// and every time it's Reset() after Stop(). +type TickerMaker func(dur time.Duration) Ticker + // Ticker is a basic ticker interface. type Ticker interface { + + // Never changes, never closes. Chan() <-chan time.Time + + // Stopping a stopped Ticker will panic. Stop() - Reset() } -// DefaultTicker wraps the stdlibs Ticker implementation. -type DefaultTicker struct { - t *time.Ticker - dur time.Duration +//---------------------------------------- +// defaultTickerMaker + +func defaultTickerMaker(dur time.Duration) Ticker { + ticker := time.NewTicker(dur) + return (*defaultTicker)(ticker) } -// NewDefaultTicker returns a new DefaultTicker -func NewDefaultTicker(dur time.Duration) *DefaultTicker { - return &DefaultTicker{ - time.NewTicker(dur), - dur, +type defaultTicker time.Ticker + +// Implements Ticker +func (t *defaultTicker) Chan() <-chan time.Time { + return t.C +} + +// Implements Ticker +func (t *defaultTicker) Stop() { + t.Stop() +} + +//---------------------------------------- +// LogicalTickerMaker + +// Construct a TickerMaker that always uses `ch`. +// It's useful for simulating a deterministic clock. +func NewLogicalTickerMaker(ch chan time.Time) TickerMaker { + return func(dur time.Duration) Ticker { + return newLogicalTicker(ch, dur) + } +} + +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 clearly need a new goroutine, for logicalTicker may have been created +// from a goroutine separate from the source. +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 *DefaultTicker) Chan() <-chan time.Time { - return t.t.C +func (t *logicalTicker) Chan() <-chan time.Time { + return t.ch // immutable } // Implements Ticker -func (t *DefaultTicker) Stop() { - t.t.Stop() - t.t = nil -} - -// Implements Ticker -func (t *DefaultTicker) Reset() { - t.t = time.NewTicker(t.dur) -} - -// ManualTicker wraps a channel that can be manually sent on -type ManualTicker struct { - ch chan time.Time -} - -// NewManualTicker returns a new ManualTicker -func NewManualTicker(ch chan time.Time) *ManualTicker { - return &ManualTicker{ - ch: ch, - } -} - -// Implements Ticker -func (t *ManualTicker) Chan() <-chan time.Time { - return t.ch -} - -// Implements Ticker -func (t *ManualTicker) Stop() { - // noop -} - -// Implements Ticker -func (t *ManualTicker) Reset() { - // noop +func (t *logicalTicker) Stop() { + close(t.quit) // it *should* panic when stopped twice. } //--------------------------------------------------------------------- /* -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. + 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 { - Ch chan time.Time + name string + ch chan time.Time + tm TickerMaker mtx sync.Mutex - name string + dur time.Duration ticker Ticker quit chan struct{} - wg *sync.WaitGroup } -// NewRepeatTimer returns a RepeatTimer with the DefaultTicker. +// NewRepeatTimer returns a RepeatTimer with a defaultTicker. func NewRepeatTimer(name string, dur time.Duration) *RepeatTimer { - ticker := NewDefaultTicker(dur) - return NewRepeatTimerWithTicker(name, ticker) + return NewRepeatTimerWithTickerMaker(name, dur, defaultTickerMaker) } -// NewRepeatTimerWithTicker returns a RepeatTimer with the given ticker. -func NewRepeatTimerWithTicker(name string, ticker Ticker) *RepeatTimer { +// NewRepeatTimerWithTicker returns a RepeatTimer with the given ticker +// maker. +func NewRepeatTimerWithTickerMaker(name string, dur time.Duration, tm TickerMaker) *RepeatTimer { var t = &RepeatTimer{ - Ch: make(chan time.Time), - ticker: ticker, - quit: make(chan struct{}), - wg: new(sync.WaitGroup), name: name, + ch: make(chan time.Time), + tm: tm, + dur: dur, + ticker: nil, + quit: nil, } - t.wg.Add(1) - go t.fireRoutine(t.ticker.Chan()) + t.reset() return t } -func (t *RepeatTimer) fireRoutine(ch <-chan time.Time) { +func (t *RepeatTimer) fireRoutine(ch <-chan time.Time, quit <-chan struct{}) { for { select { case t_ := <-ch: - t.Ch <- t_ - case <-t.quit: - // needed so we know when we can reset t.quit - t.wg.Done() + 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.Reset() - t.quit = make(chan struct{}) - t.wg.Add(1) - go t.fireRoutine(t.ticker.Chan()) +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 - } - return exists + */ + close(t.quit) } diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 98d991e9..f43cc751 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -4,66 +4,86 @@ import ( "testing" "time" - // make govet noshadow happy... - asrt "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/assert" ) -// NOTE: this only tests with the ManualTicker. +// NOTE: this only tests with the LogicalTicker. // How do you test a real-clock ticker properly? -func TestRepeat(test *testing.T) { - assert := asrt.New(test) +func TestRepeat(t *testing.T) { ch := make(chan time.Time, 100) - // tick fires cnt times on ch + 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++ { - ch <- time.Now() + lt = lt.Add(time.Second) + ch <- lt } } - tock := func(test *testing.T, t *RepeatTimer, cnt int) { + + // tock consumes Ticker.Chan() events `cnt` times. + tock := func(t *testing.T, rt *RepeatTimer, cnt int) { for i := 0; i < cnt; i++ { - after := time.After(time.Second * 2) + timeout := time.After(time.Second * 2) select { - case <-t.Ch: - case <-after: - test.Fatal("expected ticker to fire") + case _ = <-rt.Chan(): + case <-timeout: + panic("QWE") + t.Fatal("expected RepeatTimer to fire") } } done := true select { - case <-t.Ch: + case <-rt.Chan(): done = false default: } - assert.True(done) + assert.True(t, done) } - ticker := NewManualTicker(ch) - t := NewRepeatTimerWithTicker("bar", ticker) + tm := NewLogicalTickerMaker(ch) + dur := time.Duration(0) // dontcare + rt := NewRepeatTimerWithTickerMaker("bar", dur, tm) - // start at 0 - tock(test, t, 0) + // Start at 0. + tock(t, rt, 0) + tick(1) // init time - // wait for 4 periods - tick(4) - tock(test, t, 4) + 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) - // keep reseting leads to no firing + // Multiple resets leads to no firing. for i := 0; i < 20; i++ { time.Sleep(time.Millisecond) - t.Reset() + rt.Reset() } - tock(test, t, 0) - // after this, it still works normal - tick(2) - tock(test, t, 2) + // After this, it works as new. + tock(t, rt, 0) + tick(1) // init time - // after a stop, nothing more is sent - stopped := t.Stop() - assert.True(stopped) - tock(test, t, 0) + 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) - // close channel to stop counter - close(t.Ch) + // After a stop, nothing more is sent. + rt.Stop() + tock(t, rt, 0) + + // Another stop panics. + assert.Panics(t, func() { rt.Stop() }) } From 76433d904059009050393ae31c569b7f2df72350 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Mon, 25 Dec 2017 10:13:37 -0500 Subject: [PATCH 54/61] little things --- common/repeat_timer.go | 10 +++++----- common/repeat_timer_test.go | 3 +-- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 2947a916..7c529184 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -42,11 +42,11 @@ func (t *defaultTicker) Stop() { //---------------------------------------- // LogicalTickerMaker -// Construct a TickerMaker that always uses `ch`. +// Construct a TickerMaker that always uses `source`. // It's useful for simulating a deterministic clock. -func NewLogicalTickerMaker(ch chan time.Time) TickerMaker { +func NewLogicalTickerMaker(source chan time.Time) TickerMaker { return func(dur time.Duration) Ticker { - return newLogicalTicker(ch, dur) + return newLogicalTicker(source, dur) } } @@ -66,8 +66,8 @@ func newLogicalTicker(source <-chan time.Time, interval time.Duration) Ticker { return lt } -// We clearly need a new goroutine, for logicalTicker may have been created -// from a goroutine separate from the source. +// 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 diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index f43cc751..44a1a067 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -29,7 +29,6 @@ func TestRepeat(t *testing.T) { select { case _ = <-rt.Chan(): case <-timeout: - panic("QWE") t.Fatal("expected RepeatTimer to fire") } } @@ -43,7 +42,7 @@ func TestRepeat(t *testing.T) { } tm := NewLogicalTickerMaker(ch) - dur := time.Duration(0) // dontcare + dur := time.Duration(10 * time.Millisecond) // less than a second rt := NewRepeatTimerWithTickerMaker("bar", dur, tm) // Start at 0. From 558f8e77699286ffca1f59842f54160dd30d4794 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Mon, 25 Dec 2017 11:10:48 -0500 Subject: [PATCH 55/61] fix recursion --- common/repeat_timer.go | 3 ++- common/repeat_timer_test.go | 8 ++++++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 7c529184..96348bd1 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -36,7 +36,8 @@ func (t *defaultTicker) Chan() <-chan time.Time { // Implements Ticker func (t *defaultTicker) Stop() { - t.Stop() + tt := time.Ticker(*t) + tt.Stop() } //---------------------------------------- diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 44a1a067..269316bd 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -7,8 +7,12 @@ import ( "github.com/stretchr/testify/assert" ) -// NOTE: this only tests with the LogicalTicker. -// How do you test a real-clock ticker properly? +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) From a171d906110ea86c3e9e79f3e0bd6c7c7640abc2 Mon Sep 17 00:00:00 2001 From: Jae Kwon Date: Thu, 28 Dec 2017 17:37:21 -0800 Subject: [PATCH 56/61] Fix possibly incorrect usage of conversion --- common/repeat_timer.go | 3 +-- common/repeat_timer_test.go | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/common/repeat_timer.go b/common/repeat_timer.go index 96348bd1..2e6cb81c 100644 --- a/common/repeat_timer.go +++ b/common/repeat_timer.go @@ -36,8 +36,7 @@ func (t *defaultTicker) Chan() <-chan time.Time { // Implements Ticker func (t *defaultTicker) Stop() { - tt := time.Ticker(*t) - tt.Stop() + ((*time.Ticker)(t)).Stop() } //---------------------------------------- diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index 269316bd..da168707 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -31,7 +31,7 @@ func TestRepeat(t *testing.T) { for i := 0; i < cnt; i++ { timeout := time.After(time.Second * 2) select { - case _ = <-rt.Chan(): + case <-rt.Chan(): case <-timeout: t.Fatal("expected RepeatTimer to fire") } From 71f13cc071258fbcfe3fb3a3438d1a9f0ee0f4e0 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 29 Dec 2017 10:42:02 -0500 Subject: [PATCH 57/61] drop metalinter --- test.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test.sh b/test.sh index 02bdaae8..b3978d3f 100755 --- a/test.sh +++ b/test.sh @@ -2,14 +2,14 @@ set -e # run the linter -make metalinter_test +# 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 - cat profile.out >> coverage.txt - rm profile.out - fi + go test -race -coverprofile=profile.out -covermode=atomic "$d" + if [ -f profile.out ]; then + cat profile.out >> coverage.txt + rm profile.out + fi done From 92c17f3f251d51878dc866a42dc57dc09df88ac8 Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 29 Dec 2017 10:49:49 -0500 Subject: [PATCH 58/61] give test more time --- common/repeat_timer_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/repeat_timer_test.go b/common/repeat_timer_test.go index da168707..5a3a4c0a 100644 --- a/common/repeat_timer_test.go +++ b/common/repeat_timer_test.go @@ -29,11 +29,11 @@ func TestRepeat(t *testing.T) { // 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 * 2) + timeout := time.After(time.Second * 10) select { case <-rt.Chan(): case <-timeout: - t.Fatal("expected RepeatTimer to fire") + panic("expected RepeatTimer to fire") } } done := true From 35e6f11ad445cf4cb19fefadba0517a86f00b1fc Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Fri, 29 Dec 2017 11:01:37 -0500 Subject: [PATCH 59/61] changelog and version --- CHANGELOG.md | 13 +++++++++++++ version/version.go | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b679b839..fe2c2fe9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## 0.6.0 (December 29, 2017) + +BREAKING: + - [cli] remove --root + - [pubsub] add String() method to Query interface + +IMPROVEMENTS: + - [common] use a thread-safe and well seeded non-crypto rng + +BUG FIXES + - [clist] fix misuse of wait group + - [common] introduce Ticker interface and logicalTicker for better testing of timers + ## 0.5.0 (December 5, 2017) BREAKING: diff --git a/version/version.go b/version/version.go index 45222da7..6cc88728 100644 --- a/version/version.go +++ b/version/version.go @@ -1,3 +1,3 @@ package version -const Version = "0.5.0" +const Version = "0.6.0" From 9f72e25b23f3a3120af5e48b5b7520c34b88775f Mon Sep 17 00:00:00 2001 From: Ethan Buchman Date: Sat, 16 Dec 2017 00:03:40 -0500 Subject: [PATCH 60/61] readme --- README.md | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 README.md diff --git a/README.md b/README.md new file mode 100644 index 00000000..d5a11c7b --- /dev/null +++ b/README.md @@ -0,0 +1,53 @@ +# TMLIBS + +This repo is a home for various small packages. + +## autofile + +Autofile is file access with automatic log rotation. A group of files is maintained and rotation happens +when the leading file gets too big. Provides a reader for reading from the file group. + +## cli + +CLI wraps the `cobra` and `viper` packages and handles some common elements of building a CLI like flags and env vars for the home directory and the logger. + +## clist + +Clist provides a linekd list that is safe for concurrent access by many readers. + +## common + +Common provides a hodgepodge of useful functions. + +## db + +DB provides a database interface and a number of implementions, including ones using an in-memory map, the filesystem directory structure, +an implemention of LevelDB in Go, and the official LevelDB in C. + +## events + +Events is a synchronous PubSub package. + +## flowrate + +Flowrate is a fork of https://github.com/mxk/go-flowrate that added a `SetREMA` method. + +## log + +Log is a log package structured around key-value pairs that allows logging level to be set differently for different keys. + +## logger + +Logger is DEPRECATED. It's a simple wrapper around `log15`. + +## merkle + +Merkle provides a simple static merkle tree and corresponding proofs. + +## process + +Process is a simple utility for spawning OS processes. + +## pubsub + +PubSub is an asynchronous PubSub package. From a84bc2f5b26094bbd15dfefe46a2ac932fc9d557 Mon Sep 17 00:00:00 2001 From: Zach Ramsay Date: Fri, 29 Dec 2017 15:23:07 +0000 Subject: [PATCH 61/61] logger is deprecated, removed; closes #115 --- README.md | 4 --- logger/log.go | 78 --------------------------------------------------- 2 files changed, 82 deletions(-) delete mode 100644 logger/log.go diff --git a/README.md b/README.md index d5a11c7b..9ea618db 100644 --- a/README.md +++ b/README.md @@ -36,10 +36,6 @@ Flowrate is a fork of https://github.com/mxk/go-flowrate that added a `SetREMA` Log is a log package structured around key-value pairs that allows logging level to be set differently for different keys. -## logger - -Logger is DEPRECATED. It's a simple wrapper around `log15`. - ## merkle Merkle provides a simple static merkle tree and corresponding proofs. diff --git a/logger/log.go b/logger/log.go deleted file mode 100644 index 2f4faef6..00000000 --- a/logger/log.go +++ /dev/null @@ -1,78 +0,0 @@ -// DEPRECATED! Use newer log package. -package logger - -import ( - "os" - - "github.com/tendermint/log15" - . "github.com/tendermint/tmlibs/common" -) - -var mainHandler log15.Handler -var bypassHandler log15.Handler - -func init() { - resetWithLogLevel("debug") -} - -func SetLogLevel(logLevel string) { - resetWithLogLevel(logLevel) -} - -func resetWithLogLevel(logLevel string) { - // main handler - //handlers := []log15.Handler{} - mainHandler = log15.LvlFilterHandler( - getLevel(logLevel), - log15.StreamHandler(os.Stdout, log15.TerminalFormat()), - ) - //handlers = append(handlers, mainHandler) - - // bypass handler for not filtering on global logLevel. - bypassHandler = log15.StreamHandler(os.Stdout, log15.TerminalFormat()) - //handlers = append(handlers, bypassHandler) - - // By setting handlers on the root, we handle events from all loggers. - log15.Root().SetHandler(mainHandler) -} - -// See go-wire/log for an example of usage. -func MainHandler() log15.Handler { - return mainHandler -} - -func New(ctx ...interface{}) log15.Logger { - return NewMain(ctx...) -} - -func BypassHandler() log15.Handler { - return bypassHandler -} - -func NewMain(ctx ...interface{}) log15.Logger { - return log15.Root().New(ctx...) -} - -func NewBypass(ctx ...interface{}) log15.Logger { - bypass := log15.New(ctx...) - bypass.SetHandler(bypassHandler) - return bypass -} - -func getLevel(lvlString string) log15.Lvl { - lvl, err := log15.LvlFromString(lvlString) - if err != nil { - Exit(Fmt("Invalid log level %v: %v", lvlString, err)) - } - return lvl -} - -//---------------------------------------- -// Exported from log15 - -var LvlFilterHandler = log15.LvlFilterHandler -var LvlDebug = log15.LvlDebug -var LvlInfo = log15.LvlInfo -var LvlNotice = log15.LvlNotice -var LvlWarn = log15.LvlWarn -var LvlError = log15.LvlError