Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move Shutdown lock from Handler into Engines #2179

Merged
merged 3 commits into from
Oct 19, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions snow/engine/avalanche/bootstrap/bootstrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,10 @@ func (*bootstrapper) Gossip(context.Context) error {

func (b *bootstrapper) Shutdown(ctx context.Context) error {
b.Ctx.Log.Info("shutting down bootstrapper")

b.Ctx.Lock.Lock()
defer b.Ctx.Lock.Unlock()

return b.VM.Shutdown(ctx)
}

Expand Down
4 changes: 4 additions & 0 deletions snow/engine/snowman/bootstrap/bootstrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -295,6 +295,10 @@ func (*bootstrapper) Gossip(context.Context) error {

func (b *bootstrapper) Shutdown(ctx context.Context) error {
b.Ctx.Log.Info("shutting down bootstrapper")

b.Ctx.Lock.Lock()
defer b.Ctx.Lock.Unlock()

return b.VM.Shutdown(ctx)
}

Expand Down
4 changes: 4 additions & 0 deletions snow/engine/snowman/syncer/state_syncer.go
Original file line number Diff line number Diff line change
Expand Up @@ -590,6 +590,10 @@ func (*stateSyncer) Gossip(context.Context) error {

func (ss *stateSyncer) Shutdown(ctx context.Context) error {
ss.Config.Ctx.Log.Info("shutting down state syncer")

ss.Ctx.Lock.Lock()
defer ss.Ctx.Lock.Unlock()

return ss.VM.Shutdown(ctx)
}

Expand Down
4 changes: 4 additions & 0 deletions snow/engine/snowman/transitive.go
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,10 @@ func (*Transitive) Halt(context.Context) {}

func (t *Transitive) Shutdown(ctx context.Context) error {
t.Ctx.Log.Info("shutting down consensus engine")

t.Ctx.Lock.Lock()
defer t.Ctx.Lock.Unlock()

return t.VM.Shutdown(ctx)
}

Expand Down
39 changes: 17 additions & 22 deletions snow/networking/handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"errors"
"fmt"
"sync"
"sync/atomic"
"time"

"github.com/prometheus/client_golang/prometheus"
Expand Down Expand Up @@ -116,7 +117,7 @@ type handler struct {
startClosingTime time.Time
totalClosingTime time.Duration
closingChan chan struct{}
numDispatchersClosed int
numDispatchersClosed atomic.Uint32
// Closed when this handler and [engine] are done shutting down
closed chan struct{}

Expand Down Expand Up @@ -220,22 +221,24 @@ func (h *handler) selectStartingGear(ctx context.Context) (common.Engine, error)

func (h *handler) Start(ctx context.Context, recoverPanic bool) {
h.ctx.Lock.Lock()
defer h.ctx.Lock.Unlock()

gear, err := h.selectStartingGear(ctx)
if err != nil {
h.ctx.Lock.Unlock()

h.ctx.Log.Error("chain failed to select starting gear",
zap.Error(err),
)
h.shutdown(ctx)
h.shutdown(ctx, h.clock.Time())
return
}

if err := gear.Start(ctx, 0); err != nil {
err = gear.Start(ctx, 0)
h.ctx.Lock.Unlock()
if err != nil {
h.ctx.Log.Error("chain failed to start",
zap.Error(err),
)
h.shutdown(ctx)
h.shutdown(ctx, h.clock.Time())
return
}

Expand Down Expand Up @@ -326,7 +329,7 @@ func (h *handler) Stop(ctx context.Context) {
state := h.ctx.State.Get()
bootstrapper, ok := h.engineManager.Get(state.Type).Get(snow.Bootstrapping)
if !ok {
h.ctx.Log.Error("bootstrapping engine doesn't exists",
h.ctx.Log.Error("bootstrapping engine doesn't exist",
zap.Stringer("type", state.Type),
)
return
Expand Down Expand Up @@ -998,35 +1001,27 @@ func (h *handler) popUnexpiredMsg(
}
}

// Invariant: if closeDispatcher is called, Stop has already been called.
func (h *handler) closeDispatcher(ctx context.Context) {
h.ctx.Lock.Lock()
defer h.ctx.Lock.Unlock()
Comment on lines -1002 to -1003
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is basically the whole goal of the PR


h.numDispatchersClosed++
if h.numDispatchersClosed < numDispatchersToClose {
if h.numDispatchersClosed.Add(1) < numDispatchersToClose {
return
}

h.shutdown(ctx)
h.shutdown(ctx, h.startClosingTime)
}

// Note: shutdown is only called after all message dispatchers have exited.
func (h *handler) shutdown(ctx context.Context) {
// Note: shutdown is only called after all message dispatchers have exited or if
// no message dispatchers ever started.
func (h *handler) shutdown(ctx context.Context, startClosingTime time.Time) {
defer func() {
if h.onStopped != nil {
go h.onStopped()
}

h.totalClosingTime = h.clock.Time().Sub(h.startClosingTime)
h.totalClosingTime = h.clock.Time().Sub(startClosingTime)
close(h.closed)
}()

// shutdown may be called during Start, so we populate the start closing
// time here in case Stop was never called.
if h.startClosingTime.IsZero() {
h.startClosingTime = h.clock.Time()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was previously racy with Stop being called if shutdown was called from Start.

}

state := h.ctx.State.Get()
engine, ok := h.engineManager.Get(state.Type).Get(state.State)
if !ok {
Expand Down