shutdown gracefully without os.Exit #7480

yihuang · 2020-10-08T02:12:10Z

Description

Change the behaviour of signal handling to gracefully shutdown rather than os.Exit.
The purpose is to give defer function or wrapper function a chance to run after it quit.
Particularly in our case, we want to check test coverage of our externally running integration tests, we take this approach. basically we wrap the main function in a test function, and hope the main function returns normally when signaled with SIGINT/SIGTERM, so the test binary will output the coverage report.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards.
Wrote unit and integration tests
Updated relevant documentation (docs/) or specification (x/<module>/spec/)
Added relevant godoc comments.
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the Github PR explorer
Review Codecov Report in the comment section below once CI passes

codecov · 2020-10-08T04:44:34Z

Codecov Report

Merging #7480 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #7480      +/-   ##
==========================================
- Coverage   55.01%   54.82%   -0.19%     
==========================================
  Files         448      597     +149     
  Lines       30321    37996    +7675     
==========================================
+ Hits        16681    20833    +4152     
- Misses      11994    15049    +3055     
- Partials     1646     2114     +468

alessio

Makes sense to me. utACK.

alexanderbez

I don't quite follow why this approach is better. Can you provide concrete examples? Having a single signal trap that encapsulates all cleanup logic is preferable to me.

yihuang · 2020-10-08T13:55:46Z

I don't quite follow why this approach is better. Can you provide concrete examples? Having a single signal trap that encapsulates all cleanup logic is preferable to me.

The method we use to collect test coverage when doing external testing is described here. The example code is here.

Basically what we do is wrapping the main function in a test function:

func TestBincoverRunMain(t *testing.T) {
	bincover.RunTest(main)
}

And built it with go test -coverpkg=github.com/cosmos/cosmos-sdk/...,./... -c -o ./chain-maind-inst, and run it with chain-maind-inst -test.coverprofile=coverage.out start --home=..., so when the main function returns normally, the binary will save the test coverage report in coverage.out.
so we start the devnet with it, run external integration tests(in python) against it, when the testing is done, we send signal SIGTERM to quit the chain process, and hope it output the coverage report before quit, that's why we don't want it to just os.Exit.

alexanderbez · 2020-10-08T15:05:02Z

Is there a way in the TrapSignal to ensure you capture the coverage report?

yihuang · 2020-10-08T23:35:30Z

Is there a way in the TrapSignal to ensure you capture the coverage report?

The report generation is handled by go internal after the test functions returned, what I observe currently is it's not generated with os.Exit.

alexanderbez · 2020-10-09T13:52:36Z

I see. What if we added a time/sleep before os.Exit?

yihuang · 2020-10-09T14:57:21Z

I see. What if we added a time/sleep before os.Exit?

I think it won't work unless the main function returns normally, because the wrapping code don't have a chance to run.

alessio · 2020-10-09T15:14:45Z

I see. What if we added a time/sleep before os.Exit?

I think it won't work unless the main function returns normally, because the wrapping code don't have a chance to run.

And it'd be very, very ugly...

alexanderbez · 2020-10-09T16:13:08Z

server/start.go

+	if tmNode.IsRunning() {
+		_ = tmNode.Stop()
+	}

-		if apiSrv != nil {
-			_ = apiSrv.Close()
-		}
+	if cpuProfileCleanup != nil {
+		cpuProfileCleanup()
+	}

-		if grpcSrv != nil {
-			grpcSrv.Stop()
-		}
+	if apiSrv != nil {
+		_ = apiSrv.Close()
+	}

-		ctx.Logger.Info("exiting...")
-	})
+	if grpcSrv != nil {
+		grpcSrv.Stop()
+	}


Ok can we at least group these in a single cleanup function?

made it into a defer expression now.

alexanderbez · 2020-10-09T16:13:16Z

server/start.go

-		if tmNode.IsRunning() {
-			_ = tmNode.Stop()
-		}
+	WaitForQuitSignals()


We're not using the return value here.

I added the _ = now.

alexanderbez

LGTM. But @alessio, don't we want to exit with the status code resulting from WaitForQuitSignals?

yihuang · 2020-10-11T00:00:36Z

LGTM. But @alessio, don't we want to exit with the status code resulting from WaitForQuitSignals?

we might need a ErrorCode{int} for that, I can do it if we need that.

alessio · 2020-10-12T12:25:21Z

we might need a ErrorCode{int} for that, I can do it if we need that.

That'd be great

alessio · 2020-10-12T15:06:38Z

server/util.go

+	sigs := make(chan os.Signal, 1)
+	signal.Notify(sigs, syscall.SIGINT, syscall.SIGTERM)
+	sig := <-sigs
+	return ErrorCode{Code: int(sig.(syscall.Signal)) + 128}


Why adding 128?

Because that is the standard way you handle -signal process termination in UNIX (n+128)

server/util.go

alessio

Tests are failing - looks great otherwise

robert-zaremba · 2020-10-13T10:45:33Z

simapp/simd/main.go

 	"github.com/cosmos/cosmos-sdk/simapp/simd/cmd"
 )

 func main() {
 	rootCmd, _ := cmd.NewRootCmd()
 	if err := cmd.Execute(rootCmd); err != nil {
-		os.Exit(1)
+		switch e := err.(type) {


Should we log an error as well?

Exit code + logging information already provided by the underlying goroutines should do it all.

Current error output when received SIGTERM is like this:

Error: 143 Usage: chain-maind start [flags] Flags: ...

maybe add an error string into the ErrorCode?

server/util.go

alessio

looks good to me

* shutdown gracefully without os.Exit * Update server/util.go Co-authored-by: Alessio Treglia <quadrispro@ubuntu.com> Co-authored-by: Alessio Treglia <alessio@tendermint.com>

yihuang requested review from aaronc, alessio and alexanderbez as code owners October 8, 2020 02:12

yihuang force-pushed the shutdown-gracefully branch 2 times, most recently from 7d3d6bf to 32f528c Compare October 8, 2020 04:38

alessio approved these changes Oct 8, 2020

View reviewed changes

yihuang mentioned this pull request Oct 8, 2020

Problem (fix #149): no integration test coverage report crypto-org-chain/chain-main#150

Merged

alexanderbez suggested changes Oct 8, 2020

View reviewed changes

alexanderbez suggested changes Oct 9, 2020

View reviewed changes

yihuang force-pushed the shutdown-gracefully branch 2 times, most recently from af475f3 to 0834b90 Compare October 10, 2020 03:04

yihuang requested a review from alexanderbez October 10, 2020 03:05

yihuang force-pushed the shutdown-gracefully branch 2 times, most recently from df74d17 to 696d612 Compare October 10, 2020 03:06

alexanderbez approved these changes Oct 10, 2020

View reviewed changes

yihuang force-pushed the shutdown-gracefully branch 3 times, most recently from 78e74cf to 3619164 Compare October 12, 2020 14:43

alessio reviewed Oct 12, 2020

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

alessio suggested changes Oct 12, 2020

View reviewed changes

yihuang force-pushed the shutdown-gracefully branch from b684f8e to 84c1387 Compare October 12, 2020 21:16

shutdown gracefully without os.Exit

51e2a6d

yihuang force-pushed the shutdown-gracefully branch from 84c1387 to 51e2a6d Compare October 12, 2020 21:44

robert-zaremba reviewed Oct 13, 2020

View reviewed changes

alexanderbez requested a review from alessio October 14, 2020 12:50

alessio reviewed Oct 14, 2020

View reviewed changes

server/util.go Outdated Show resolved Hide resolved

Update server/util.go

78599d4

alessio approved these changes Oct 14, 2020

View reviewed changes

Merge branch 'master' into shutdown-gracefully

f691fa0

alessio added the A:automerge Automatically merge PR once all prerequisites pass. label Oct 14, 2020

mergify bot merged commit f260ca4 into cosmos:master Oct 14, 2020

amaury1093 added the backport/0.40.x (Stargate) label Oct 23, 2020

clevinson mentioned this pull request Oct 29, 2020

[backports]: v0.40.0-rc2 Cherry-picks #7734

Closed

45 tasks

clevinson mentioned this pull request Mar 5, 2021

fix(app): exit with error code and use regen env prefix regen-network/regen-ledger#285

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shutdown gracefully without os.Exit #7480

shutdown gracefully without os.Exit #7480

yihuang commented Oct 8, 2020 •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading

alessio left a comment

alexanderbez left a comment

yihuang commented Oct 8, 2020 •

edited

Loading

alexanderbez commented Oct 8, 2020

yihuang commented Oct 8, 2020 •

edited

Loading

alexanderbez commented Oct 9, 2020

yihuang commented Oct 9, 2020

alessio commented Oct 9, 2020

alexanderbez Oct 9, 2020

yihuang Oct 10, 2020

alexanderbez Oct 9, 2020

yihuang Oct 10, 2020

alexanderbez left a comment

yihuang commented Oct 11, 2020 •

edited

Loading

alessio commented Oct 12, 2020

alessio Oct 12, 2020

robert-zaremba Oct 13, 2020

alessio Oct 13, 2020

alessio left a comment

robert-zaremba Oct 13, 2020

alessio Oct 13, 2020

yihuang Oct 14, 2020 •

edited

Loading

alessio left a comment

shutdown gracefully without os.Exit #7480

shutdown gracefully without os.Exit #7480

Conversation

yihuang commented Oct 8, 2020 • edited Loading

Description

codecov bot commented Oct 8, 2020 • edited Loading

Codecov Report

alessio left a comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

yihuang commented Oct 8, 2020 • edited Loading

alexanderbez commented Oct 8, 2020

yihuang commented Oct 8, 2020 • edited Loading

alexanderbez commented Oct 9, 2020

yihuang commented Oct 9, 2020

alessio commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

yihuang commented Oct 11, 2020 • edited Loading

alessio commented Oct 12, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuang Oct 14, 2020 • edited Loading

Choose a reason for hiding this comment

alessio left a comment

Choose a reason for hiding this comment

yihuang commented Oct 8, 2020 •

edited

Loading

codecov bot commented Oct 8, 2020 •

edited

Loading

yihuang commented Oct 8, 2020 •

edited

Loading

yihuang commented Oct 8, 2020 •

edited

Loading

yihuang commented Oct 11, 2020 •

edited

Loading

yihuang Oct 14, 2020 •

edited

Loading