diff --git a/consensus/consensus_test.go b/consensus/consensus_test.go index ccd8ff03..f5e33f5c 100644 --- a/consensus/consensus_test.go +++ b/consensus/consensus_test.go @@ -3,6 +3,7 @@ package consensus_test import ( "context" "testing" + "time" "github.com/golang/mock/gomock" "github.com/relab/hotstuff" @@ -19,7 +20,7 @@ func TestVote(t *testing.T) { ctrl := gomock.NewController(t) bl := testutil.CreateBuilders(t, ctrl, n) cs := mocks.NewMockConsensus(ctrl) - bl[0].Add(synchronizer.New(testutil.FixedTimeout(1000)), cs) + bl[0].Add(synchronizer.New(testutil.FixedTimeout(1*time.Millisecond)), cs) hl := bl.Build() hs := hl[0] diff --git a/synchronizer/synchronizer.go b/synchronizer/synchronizer.go index 1aa9714c..32aaa6ad 100644 --- a/synchronizer/synchronizer.go +++ b/synchronizer/synchronizer.go @@ -36,7 +36,7 @@ type Synchronizer struct { lastTimeout *hotstuff.TimeoutMsg duration ViewDuration - timer *time.Timer + timer oneShotTimer // map of collected timeout messages per view timeouts map[hotstuff.View]map[hotstuff.ID]hotstuff.TimeoutMsg @@ -89,17 +89,34 @@ func New(viewDuration ViewDuration) modules.Synchronizer { currentView: 1, duration: viewDuration, - timer: time.AfterFunc(0, func() {}), // dummy timer that will be replaced after start() is called + timer: oneShotTimer{time.AfterFunc(0, func() {})}, // dummy timer that will be replaced after start() is called timeouts: make(map[hotstuff.View]map[hotstuff.ID]hotstuff.TimeoutMsg), } } +// A oneShotTimer is a timer that should not be reused. +type oneShotTimer struct { + timerDoNotUse *time.Timer +} + +func (t oneShotTimer) Stop() bool { + return t.timerDoNotUse.Stop() +} + func (s *Synchronizer) startTimeoutTimer() { - s.timer = time.AfterFunc(s.duration.Duration(), func() { + // Store the view in a local variable to avoid calling s.View() in the closure below, + // thus avoiding a data race. + view := s.View() + // It is important that the timer is NOT reused because then the view would be wrong. + s.timer = oneShotTimer{time.AfterFunc(s.duration.Duration(), func() { // The event loop will execute onLocalTimeout for us. - s.eventLoop.AddEvent(TimeoutEvent{s.View()}) - }) + s.eventLoop.AddEvent(TimeoutEvent{view}) + })} +} + +func (s *Synchronizer) stopTimeoutTimer() { + s.timer.Stop() } // Start starts the synchronizer with the given context. @@ -108,7 +125,7 @@ func (s *Synchronizer) Start(ctx context.Context) { go func() { <-ctx.Done() - s.timer.Stop() + s.stopTimeoutTimer() }() // start the initial proposal @@ -305,7 +322,7 @@ func (s *Synchronizer) AdvanceView(syncInfo hotstuff.SyncInfo) { return } - s.timer.Stop() + s.stopTimeoutTimer() if !timeout { s.duration.ViewSucceeded() @@ -318,8 +335,7 @@ func (s *Synchronizer) AdvanceView(syncInfo hotstuff.SyncInfo) { s.lastTimeout = nil s.duration.ViewStarted() - duration := s.duration.Duration() - s.timer.Reset(duration) + s.startTimeoutTimer() s.logger.Debugf("advanced to view %d", newView) s.eventLoop.AddEvent(ViewChangeEvent{View: newView, Timeout: timeout}) diff --git a/twins/network.go b/twins/network.go index 1c2b677d..3f3e029c 100644 --- a/twins/network.go +++ b/twins/network.go @@ -143,7 +143,7 @@ func (n *Network) createTwinsNodes(nodes []NodeID, _ Scenario, consensusName str consensus.New(consensusModule), consensus.NewVotingMachine(), crypto.NewCache(ecdsa.New(), 100), - synchronizer.New(FixedTimeout(0)), + synchronizer.New(FixedTimeout(1*time.Millisecond)), logging.NewWithDest(&node.log, fmt.Sprintf("r%dn%d", nodeID.ReplicaID, nodeID.NetworkID)), // twins-specific: &configuration{network: n, node: node},