feat: cleanup server start function #16208
Conversation
github-prbot
requested review from
a team,
aaronc and
likhita-809
and removed request for
a team
| @@ -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) | |||
There was a problem hiding this comment.
start the signal listening goroutine in the group, so the group is never empty.
| g.Go(func() error { | ||
| <-ctx.Done() | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
the signal listening goroutine is started in the group, so the group is never empty.
| if traceWriterCleanup != nil { | ||
| traceWriterCleanup() | ||
| } | ||
| }() |
There was a problem hiding this comment.
the big defer function is broken up into smaller pieces, and put right after the respective resource is created.
| }() | ||
|
|
||
| return <-errCh | ||
| return callbackFn() |
| nodeKey, err := p2p.LoadOrGenNodeKey(cmtCfg.NodeKeyFile()) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
the code is moved according to the TODO note.
|
Have these changes been tested on a node? Preferably in various conditions (gRPC only, API and/or gRPC enabled/disabled). |
yihuang
added
backport/0.46.x
I just tried on a local devnet node (starting the node in foreground, and interrupted with Ctl+C), with different configs and |
|
should we close this in favour of #16238 or wait for it be merged? |
|
This PR has another improvement not in #16238, for getting the errorgroup handled better than today. (Putting listenForQuitSignals into the errorgroup, and removing the confusing dummy errorgroup wait) That should def get in, but maybe it makes more sense after 16238 is merged? |
|
I think it's easier that @ValarDragon take what's useful into #16238, and we can close this one. |
|
Ok! |
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change