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.

Sign up for GitHub

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

feat: cleanup server start function #16208

Closed
wants to merge 2 commits into from
Closed

feat: cleanup server start function #16208

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3406075
feat: cleanup server start function
yihuang May 18, 2023
63315db
cleanup
yihuang May 18, 2023
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
56 changes: 22 additions & 34 deletions server/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,7 @@ func startStandAlone(svrCtx *Context, appCreator types.AppCreator) error {
g, ctx := errgroup.WithContext(ctx)

// listen for quit signals so the calling parent process can gracefully exit
ListenForQuitSignals(cancelFn, svrCtx.Logger)
ListenForQuitSignals(g, cancelFn, svrCtx.Logger)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

start the signal listening goroutine in the group, so the group is never empty.


g.Go(func() error {
if err := svr.Start(); err != nil {
Expand Down Expand Up @@ -285,14 +285,14 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types.
if err != nil {
return err
}
defer traceWriterCleanup()

app := appCreator(svrCtx.Logger, db, traceWriter, svrCtx.Viper)

// TODO: Move this to only be done if were launching the node. (So not in GRPC-only mode)
nodeKey, err := p2p.LoadOrGenNodeKey(cmtCfg.NodeKeyFile())
if err != nil {
return err
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the code is moved according to the TODO note.

defer func() {
if err := app.Close(); err != nil {
svrCtx.Logger.Error("error closing application", "err", err)
}
}()

var (
tmNode *node.Node
Expand All @@ -304,11 +304,17 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types.
svrCfg.GRPC.Enable = true
} else {
svrCtx.Logger.Info("starting node with ABCI CometBFT in-process")
tmNode, err = startCmtNode(cmtCfg, nodeKey, app, svrCtx)
tmNode, err = startCmtNode(cmtCfg, app, svrCtx)
if err != nil {
return err
}

defer func() {
if err := tmNode.Stop(); err != nil {
svrCtx.Logger.Error("error stopping CometBFT node", "err", err)
}
}()

// Add the tx service to the gRPC router. We only need to register this
// service if API or gRPC is enabled, and avoid doing so in the general
// case, because it spawns a new local CometBFT RPC client.
Expand All @@ -334,7 +340,7 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types.
g, ctx := errgroup.WithContext(ctx)

// listen for quit signals so the calling parent process can gracefully exit
ListenForQuitSignals(cancelFn, svrCtx.Logger)
ListenForQuitSignals(g, cancelFn, svrCtx.Logger)

grpcSrv, clientCtx, err := startGrpcServer(ctx, g, svrCfg.GRPC, clientCtx, svrCtx, app)
if err != nil {
Expand All @@ -353,34 +359,21 @@ func startInProcess(svrCtx *Context, clientCtx client.Context, appCreator types.
return g.Wait()
}

// In case the operator has both gRPC and API servers disabled, there is
// nothing blocking this root process, so we need to block manually, so we'll
// create an empty blocking loop.
g.Go(func() error {
<-ctx.Done()
return nil
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the signal listening goroutine is started in the group, so the group is never empty.


// deferred cleanup function
// TODO: Make a generic cleanup function that takes in several func(), and runs them all.
// then we defer that.
defer func() {
if tmNode != nil && tmNode.IsRunning() {
_ = tmNode.Stop()
_ = app.Close()
}

if traceWriterCleanup != nil {
traceWriterCleanup()
}
}()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the big defer function is broken up into smaller pieces, and put right after the respective resource is created.


// wait for signal capture and gracefully return
return g.Wait()
}

// TODO: Move nodeKey into being created within the function.
func startCmtNode(cfg *cmtcfg.Config, nodeKey *p2p.NodeKey, app types.Application, svrCtx *Context) (tmNode *node.Node, err error) {
func startCmtNode(cfg *cmtcfg.Config, app types.Application, svrCtx *Context) (tmNode *node.Node, err error) {
nodeKey, err := p2p.LoadOrGenNodeKey(cfg.NodeKeyFile())
if err != nil {
return nil, err
}

tmNode, err = node.NewNode(
cfg,
pvm.LoadOrGenFilePV(cfg.PrivValidatorKeyFile(), cfg.PrivValidatorStateFile()),
Expand Down Expand Up @@ -566,12 +559,7 @@ func wrapCPUProfile(svrCtx *Context, callbackFn func() error) error {
}()
}

errCh := make(chan error)
go func() {
errCh <- callbackFn()
}()

return <-errCh
return callbackFn()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

equivalent code.

}

// emitServerInfoMetrics emits server info related metrics using application telemetry.
Expand Down
8 changes: 5 additions & 3 deletions server/util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/spf13/cobra"
"github.com/spf13/pflag"
"github.com/spf13/viper"
"golang.org/x/sync/errgroup"

"cosmossdk.io/log"
"cosmossdk.io/store"
Expand Down Expand Up @@ -373,16 +374,17 @@ func ExternalIP() (string, error) {
//
// Note, this performs a non-blocking process so the caller must ensure the
// corresponding context derived from the cancelFn is used correctly.
func ListenForQuitSignals(cancelFn context.CancelFunc, logger log.Logger) {
func ListenForQuitSignals(g *errgroup.Group, cancelFn context.CancelFunc, logger log.Logger) {
sigCh := make(chan os.Signal, 1)
signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)

go func() {
g.Go(func() error {
sig := <-sigCh
cancelFn()

logger.Info("caught signal", "signal", sig.String())
}()
return nil
})
}

// GetAppDBBackend gets the backend type to use for the application DBs.
Expand Down