POS-86: Graceful shutdown of Heimdall deamon, REST and Bridge #753

momoshell · 2021-12-02T20:09:13Z

Graceful shutdown of Heimdall, REST, and Bridge are all initiated by the same context signaled by the OS on interrupt and program termination
Necessary code duplications are due to the private functions and types from Cosmos SDK that needed changes to accommodate shutdowns

codecov-commenter · 2021-12-03T09:44:14Z

Codecov Report

Merging #753 (11e8dbd) into v2 (6a93c08) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##               v2     #753   +/-   ##
=======================================
  Coverage   72.88%   72.88%           
=======================================
  Files          49       49           
  Lines        3507     3507           
=======================================
  Hits         2556     2556           
  Misses        723      723           
  Partials      228      228

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6a93c08...11e8dbd. Read the comment docs.

vcastellm

Git history seems to have garbage, can you rebase with master?

sh3ll3x3c · 2021-12-03T13:31:25Z

We'll most definitely have to squash the commits...

momoshell · 2021-12-03T13:41:17Z

@vcastellm not sure about master though, since this was done through V2 fork, hence all the noise.
in the future, forks wont be used any more

@sh3ll3x3c agree, definitely

…ing for sync.

ferranbt

Hey, it turns out that this "small" change include a lot of code changes with respect other PRs. I would like to propose another way to handle the graceful shutdown.

Now, what you do is to have some "start" functions that setup the service (i.e. bridge, rest, daemon) and wait for it to finish. Then, you need to pass around the ctx so that they know when to stop.

Instead, something I usually do is for these "start" functions to only start (no wait to finish) and return some structs that export a Close function. Then, it is up to the functions creating and running the services (in this case, the cli commands) to start everything, wait for interrupt signal and Close every service. This Close function can be blocking and wait for it to finish everything.

ferranbt · 2021-12-06T08:59:00Z

bridge/cmd/start.go

@@ -28,6 +30,105 @@ const (
 	waitDuration = 1 * time.Minute
 )

+// StartBridgeWithCtx starts bridge service and is able to shutdow gracefully
+// returns service errors, if any
+func StartBridgeWithCtx(shutdownCtx context.Context) error {


Is this doing the same thing as StartBridge does? I see a lot of repeated code. If so, please merge both functions together.

The function signature is changed, StartBridgeWithCtx takes context instead of bool flag, and returns an error so it could be used with error groups properly. And the logic was changed so we would be provided with better overall control.

StartBridge was left there since it's being used with the package init function, wrapped inside of a GetStartCmd. I think this is a support for older commands.

Also, could you please explain what do we gain from returned structs? Are we really sure we need structs here? Could easily return Close function itself, but that's not the point here.
Passing context enables us to initiate shutdown across multiple running requests or commands, simultaneously.

Anyway, as docs say: the same Context may be passed to functions running in different goroutines; Contexts are safe for simultaneous use by multiple goroutines.
This is the exact thing we need here for a graceful shutdown.

Keep in mind that our services, that were affected by these changes, also block and wait for interrupt signals.

ferranbt · 2021-12-06T09:03:44Z

cmd/heimdalld/main.go

@@ -98,12 +102,14 @@ func GetSignerInfo(pub crypto.PubKey, priv []byte, cdc *codec.Codec) ValidatorAc

 func main() {
 	cdc := app.MakeCodec()
-	ctx := server.NewDefaultContext()
+	serverCtx := server.NewDefaultContext()


This variable change is creating a lot of new code.

This is passed to only two functions from the main itself, not sure I get this comment.

Also, we could merge server context and standard context with context.WithValue, where server context would be nested.

ferranbt · 2021-12-06T09:07:20Z

server/root.go

 }

 // ServeCommands will generate a long-running rest server
 // (aka Light Client Daemon) that exposes functionality similar
 // to the cli, but over rest
-func ServeCommands(cdc *codec.Codec, registerRoutesFn func(*lcd.RestServer)) *cobra.Command {
+func ServeCommands(shutdownCtx ctx.Context, cdc *codec.Codec, registerRoutesFn func(ctx client.CLIContext, mux *mux.Router)) *cobra.Command {


I do not think we should pass this shutdownCtx to each command because the shutdown signal should be something specific to each execution of a command, this is, something to be created inside the Run command functions.

Could we do:

RunE: func(cmd *cobra.Command, args []string) error { shutdownCtx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() err := StartRestServer(shutdownCtx, cdc, registerRoutesFn, restCh) }

In this case, each command would have created a context in its scope, where needed of course?

I could be wrong, but when we run our binary, each cmd should execute with the same context, since our main function is responsible for each execution of our defined commands?

Threading context down is a standard mechanism, not sure what I'm getting wrong here.

ferranbt · 2021-12-06T09:08:22Z

server/root.go

-	registerRoutesFn(rs)
+const shutdownTimeout = 10 * time.Second
+
+func StartRestServer(mainCtx ctx.Context, cdc *codec.Codec, registerRoutesFn func(ctx client.CLIContext, mux *mux.Router), restCh chan struct{}) error {


Is there any reason to call it mainCtx and not just ctx?

ok, done, fairly simple to change

ferranbt · 2021-12-06T09:09:42Z

server/root.go

@@ -66,11 +219,11 @@ func ServeCommands(cdc *codec.Codec, registerRoutesFn func(*lcd.RestServer)) *co
 }

 // RegisterRoutes register routes of all modules
-func RegisterRoutes(rs *lcd.RestServer) {


Why are we changing this?

That RestServer struct is imported from Cosmos SDK. For us, to be able to shut down the REST server properly, we needed access to the listener field, which is private in this case.
Not copying the RestServer struct from the package gave us control, and as an added bonus we got that logger that was also private.

Now, since the RestServer was not used in our logic for starting the server itself, the signature of RegisterRoutes was changed accordingly. And there is no need for a whole struct to be passed, since RegisterRoutes needs only two params.

sh3ll3x3c · 2021-12-07T10:02:45Z

@ferranbt we've cleaned up what was discussed, should be good to go now...

0xSasaPrsic · 2021-12-07T14:10:51Z

Hey @ferranbt we spotted a bug during heimdall initialization so will make that fix in this PR.

0xSasaPrsic · 2021-12-08T12:18:53Z

@ferranbt we made fix to heimdall initialization so it should be ready for review again.

momoshell requested review from vcastellm and ferranbt December 2, 2021 20:09

vcastellm reviewed Dec 3, 2021

View reviewed changes

0xSasaPrsic and others added 13 commits December 3, 2021 17:54

POS-50: Run Rest and Bridge service alongside the Heimdall daemon.

1f8751c

Added graceful stop for rest server

09e4f51

New function for starting Bridge with shutdown

d2be507

Heimdall, Bridge and REST can shutdown

b465496

Removed loop

5add5f7

Added graceful stop for rest server

4303a3a

New function for starting Bridge with shutdown

3987965

Heimdall, Bridge and REST can shutdown

2fd0b88

Change order of service startup and add shutdown to bridge while wait…

7448ece

…ing for sync.

Bridge process order of shutdown.

1765b92

Bridge process order of shutdown.

c634394

Removed err group

f3bc0ac

Fixed fork rebase

4e6bcd4

0xSasaPrsic force-pushed the pos-86 branch from 4a4ed59 to 4e6bcd4 Compare December 3, 2021 18:04

ferranbt suggested changes Dec 6, 2021

View reviewed changes

Renamed passed ctx variable

52e0532

momoshell force-pushed the pos-86 branch from 342a068 to 9e4a6c9 Compare December 7, 2021 09:49

Renamed ctx variable

022bcea

momoshell force-pushed the pos-86 branch from 9e4a6c9 to 022bcea Compare December 7, 2021 09:56

Nemanja0x added 2 commits December 8, 2021 09:42

Bridge flags fixed

a2a8888

Bridge flags fixed

b201fc9

Nemanja0x force-pushed the pos-86 branch from a2a8888 to b201fc9 Compare December 8, 2021 09:30

0xSasaPrsic added 2 commits December 8, 2021 10:39

Merge remote-tracking branch 'origin/pos-86' into pos-86

60fecf1

Tidy comments.

11e8dbd

ferranbt approved these changes Dec 8, 2021

View reviewed changes

momoshell merged commit 9d918a9 into maticnetwork:v2 Dec 8, 2021

momoshell deleted the pos-86 branch December 8, 2021 16:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

POS-86: Graceful shutdown of Heimdall deamon, REST and Bridge #753

POS-86: Graceful shutdown of Heimdall deamon, REST and Bridge #753

momoshell commented Dec 2, 2021

codecov-commenter commented Dec 3, 2021 •

edited

Loading

vcastellm left a comment

sh3ll3x3c commented Dec 3, 2021

momoshell commented Dec 3, 2021 •

edited

Loading

ferranbt left a comment

ferranbt Dec 6, 2021

momoshell Dec 6, 2021 •

edited

Loading

ferranbt Dec 6, 2021

momoshell Dec 6, 2021 •

edited

Loading

ferranbt Dec 6, 2021

momoshell Dec 6, 2021

ferranbt Dec 6, 2021

momoshell Dec 6, 2021

ferranbt Dec 6, 2021

momoshell Dec 6, 2021 •

edited

Loading

sh3ll3x3c commented Dec 7, 2021 •

edited

Loading

0xSasaPrsic commented Dec 7, 2021

0xSasaPrsic commented Dec 8, 2021

POS-86: Graceful shutdown of Heimdall deamon, REST and Bridge #753

POS-86: Graceful shutdown of Heimdall deamon, REST and Bridge #753

Conversation

momoshell commented Dec 2, 2021

codecov-commenter commented Dec 3, 2021 • edited Loading

Codecov Report

vcastellm left a comment

Choose a reason for hiding this comment

sh3ll3x3c commented Dec 3, 2021

momoshell commented Dec 3, 2021 • edited Loading

ferranbt left a comment

Choose a reason for hiding this comment

ferranbt Dec 6, 2021

Choose a reason for hiding this comment

momoshell Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

ferranbt Dec 6, 2021

Choose a reason for hiding this comment

momoshell Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

ferranbt Dec 6, 2021

Choose a reason for hiding this comment

momoshell Dec 6, 2021

Choose a reason for hiding this comment

ferranbt Dec 6, 2021

Choose a reason for hiding this comment

momoshell Dec 6, 2021

Choose a reason for hiding this comment

ferranbt Dec 6, 2021

Choose a reason for hiding this comment

momoshell Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

sh3ll3x3c commented Dec 7, 2021 • edited Loading

0xSasaPrsic commented Dec 7, 2021

0xSasaPrsic commented Dec 8, 2021

codecov-commenter commented Dec 3, 2021 •

edited

Loading

momoshell commented Dec 3, 2021 •

edited

Loading

momoshell Dec 6, 2021 •

edited

Loading

momoshell Dec 6, 2021 •

edited

Loading

momoshell Dec 6, 2021 •

edited

Loading

sh3ll3x3c commented Dec 7, 2021 •

edited

Loading